Re: [PATCH net-next v3 1/2] driver core: auxiliary bus: show auxiliary device IRQs

[Shay Drori <shayd@nvidia.com>]

On 10/05/2024 16:07, Przemek Kitszel wrote:
> External email: Use caution opening links or attachments
> 
> 
> please not that v4+ is already being discussed
> 
> On 5/8/24 13:33, Shay Drori wrote:
>> On 08/05/2024 12:34, Przemek Kitszel wrote:
> 
> // ...
> 
>>>> +
>>>> +/* Auxiliary devices can share IRQs. Expose to user whether the
>>>> provided IRQ is
>>>> + * shared or exclusive.
>>>> + */
>>>> +static ssize_t auxiliary_irq_mode_show(struct device *dev,
>>>> +                                    struct device_attribute *attr,
>>>> char *buf)
>>>> +{
>>>> +     struct auxiliary_irq_info *info =
>>>> +             container_of(attr, struct auxiliary_irq_info, 
>>>> sysfs_attr);
>>>> +
>>>> +     if (refcount_read(xa_load(&irqs, info->irq)) > 1)
>>>
>>> I didn't checked if it is possible with current implementation, but
>>> please imagine a scenario where user open()'s sysfs file, then triggers
>>> operation to remove irq (to call auxiliary_irq_destroy()), and only then
>>> read()'s sysfs contents, what results in nullptr dereference (xa_load()
>>> returning NULL). Splitting the code into two if statements would resolve
>>> this issue.
>>
>> the first function in auxiliary_irq_destroy() is removing the sysfs.
>> I don't see how after that user can read() the sysfs...
> 
> Let me illustrate, but with my running kernel instead of your series:
> # strace cat /sys/class/net/enp0s31f6/duplex 2>&1 | grep -e open -e read
> yields (among others):
> openat(AT_FDCWD, "/sys/class/net/enp0s31f6/duplex", O_RDONLY) = 3
> read(3, "full\n", 131072)               = 5
> 
> And now imagine that other, concurrent user app or any HW event triggers
> this IRQ removal (resulting with xarray entry removed (!), likely sysfs
> attr refcount dropped to 0 [A], so new open()s will be declined, but
> that is irrelevant).
> My assumption is that, until close()d, user is free to call read() on
> fd received from openat(), but it's possible that xa_load() would return
> NULL (because of [A] above).
> 
>>
>>>
>>>> +             return sysfs_emit(buf, "%s\n", "shared");
>>>> +     else
>>>> +             return sysfs_emit(buf, "%s\n", "exclusive");
>>>> +}
>>>> +
>>>> +static void auxiliary_irq_destroy(int irq)
>>>> +{
>>>> +     refcount_t *ref;
>>>> +
>>>> +     xa_lock(&irqs);
>>>> +     ref = xa_load(&irqs, irq);
>>>> +     if (refcount_dec_and_test(ref)) {
>>>> +             __xa_erase(&irqs, irq);
>>>> +             kfree(ref);
>>>> +     }
>>>> +     xa_unlock(&irqs);
>>>> +}
>>>> +
>>>> +static int auxiliary_irq_create(int irq)
>>>> +{
>>>> +     refcount_t *ref;
>>>> +     int ret = 0;
>>>> +
>>>> +     mutex_lock(&irqs_lock);
>>>
>>> [1] xa_lock() instead ...
>>>
>>>> +     ref = xa_load(&irqs, irq);
>>>> +     if (ref && refcount_inc_not_zero(ref))
>>>> +             goto out;
>>>
>>> `&& refcount_inc_not_zero()` here means: leak memory and wreak havoc on
>>> saturation, instead the logic should be:
>>>         if (ref) {
>>>                 refcount_inc(ref);
>>>                 goto out;
>>>         }
>>>
> 
> 
> <digression>
> 
>>> anyway allocating under a lock taken is not the best idea in general,
>>> although xarray API somehow encourages this -
>>
>>> alternative is to
>>> preallocate and free when not used, or some lock dance that will be easy
>>> to get wrong - and that's the raison d'etre of xa_reserve() :)
>>
>> I don't understand what you picture here?
> 
> Here I was digressing, sorry for not marking it clearly as that.
> IMO xarray API need an extension to make this and similar use case
> easier to code right. I will CC you ofc.
> </digression>
> 
>> xa_reserve() can drop the lock while allocating the xa_entry, so how it
>> will help?
>>
>>>
>>>> +
>>>> +     ref = kzalloc(sizeof(*ref), GFP_KERNEL);
>>>> +     if (!ref) {
>>>> +             ret = -ENOMEM;
>>>> +             goto out;
>>>> +     }
>>>> +
>>>> +     refcount_set(ref, 1);
>>>> +     ret = xa_insert(&irqs, irq, ref, GFP_KERNEL);
>>>
>>> [2] ... then __xa_insert() here
>>
>> __xa_insert() can drop the lock as well...
> 
> Thank you for pointing it to me.
> 
> Let's move future discussion on this series to your newer submissions.

thanks for the quick reviews :)
lets continue in the v4 series.

> 
> // ...