Re: [PATCH v4 2/3] mm: handle poisoning of pfn without struct pages

[Ankit Agrawal]

Thanks Andrew for the comments.

>> +int register_pfn_address_space(struct pfn_address_space *pfn_space)
>> +{
>> +     if (!pfn_space)
>> +             return -EINVAL;
>
> I suggest this be removed - make register_pfn_address_space(NULL)
> illegal and let the punishment be an oops.

Yes, will remove it.

>> +static void add_to_kill_pfn(struct task_struct *tsk,
>> +                         struct vm_area_struct *vma,
>> +                         struct list_head *to_kill,
>> +                         unsigned long pfn)
>> +{
>> +     struct to_kill *tk;
>> +
>> +     tk = kmalloc(sizeof(*tk), GFP_ATOMIC);
>> +     if (!tk)
>> +             return;
>
> This is unfortunate.  GFP_ATOMIC is unreliable and we silently behave
> as if it worked OK.

Got it. I'll mark this as a failure case.


> We could play games here to make the GFP_ATOMIC allocation unnecessary,
> but nasty.  Allocate the to_kill* outside the rcu_read_lock, pass that
> pointer into add_to_kill_pfn().  If add_to_kill_pfn()'s
> kmalloc(GFP_ATOMIC) failed, add_to_kill_pfn() can then consume the
> caller's to_kill*.  Then the caller can drop the lock, allocate a new
> to_kill* then restart the scan.  And teach add_to_kill_pfn() to not
> re-add tasks which are already on the list.  Ugh.
>
> At the very very least we should tell the user that the kernel goofed
> and that one of their processes won't be getting killed.

Thanks for the suggestion. As mentioned above I'll mark the kmalloc
allocation error as a failure and can put a log message there.

>> +     scoped_guard(mutex, &pfn_space_lock) {
>> +             bool mf_handled = false;
>> +
>> +             /*
>> +              * Modules registers with MM the address space mapping to the device memory they
>> +              * manage. Iterate to identify exactly which address space has mapped to this
>> +              * failing PFN.
>
> We're quite lenient about >80 columns nowadays, but overflowing 80 for
> a block comment is rather needless.

Yes. Since it passed through the strict checkpatch.pl check, I didn't notice.
I'll fix it.