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

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Jul 05, 2024 at 08:35:33AM +0300, Shay Drori wrote:
> 
> 
> On 04/07/2024 13:41, Greg KH wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, Jul 03, 2024 at 10:38:57AM +0300, Shay Drory wrote:
> > > +/**
> > > + * auxiliary_device_sysfs_irq_add - add a sysfs entry for the given IRQ
> > > + * @auxdev: auxiliary bus device to add the sysfs entry.
> > > + * @irq: The associated interrupt number.
> > > + *
> > > + * This function should be called after auxiliary device have successfully
> > > + * received the irq.
> > > + * The driver is responsible to add a unique irq for the auxiliary device. The
> > > + * driver can invoke this function from multiple thread context safely for
> > > + * unique irqs of the auxiliary devices. The driver must not invoke this API
> > > + * multiple times if the irq is already added previously.
> > > + *
> > > + * Return: zero on success or an error code on failure.
> > > + */
> > > +int auxiliary_device_sysfs_irq_add(struct auxiliary_device *auxdev, int irq)
> > > +{
> > > +     struct auxiliary_irq_info *info __free(kfree) = NULL;
> > > +     struct device *dev = &auxdev->dev;
> > > +     char *name __free(kfree) = NULL;
> > > +     int ret;
> > > +
> > > +     ret = auxiliary_irq_dir_prepare(auxdev);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     info = kzalloc(sizeof(*info), GFP_KERNEL);
> > > +     if (!info)
> > > +             return -ENOMEM;
> > > +
> > > +     sysfs_attr_init(&info->sysfs_attr.attr);
> > > +     name = kasprintf(GFP_KERNEL, "%d", irq);
> > > +     if (!name)
> > > +             return -ENOMEM;
> > > +
> > > +     ret = xa_insert(&auxdev->irqs, irq, info, GFP_KERNEL);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     info->sysfs_attr.attr.name = name;
> > > +     ret = sysfs_add_file_to_group(&dev->kobj, &info->sysfs_attr.attr,
> > > +                                   auxiliary_irqs_group.name);
> > > +     if (ret)
> > > +             goto sysfs_add_err;
> > > +
> > > +     info->sysfs_attr.attr.name = no_free_ptr(name);
> > 
> > This assignment of a name AFTER it has been created is odd.  I think I
> > know why you are doing this, but please make it obvious and perhaps
> > solve it in a cleaner way.
> 
> I am doing it since I want the name memory to be freed in case of
> sysfs_add_file_to_group() fails.
> I don’t see a cleaner way available with cleanup.h.
> 
> > Assigning this "deep" in a sysfs structure is not ok.
> 
> when creating sysfs dynamically, there isn't a cleaner way to assign the
> name memory.
> The closest and exact same use case for pci irq sysfs which uses dynamic
> sysfs is msi_sysfs_populate_desc().
> It does not use cleanup.h but still has to assign.
> I Don’t have any other ideas on how to implement it any more elegantly
> with cleanup.h.
> Do you prefer to assign it before sysfs_add_file_to_group() similar to
> msi_sysfs_populate_desc() and avoid cleanup.h for now?

No, what msi_sysfs_populate_desc() does is not good, the only objection
here is the assignment after-the-fact you are doing just to work around
cleanup.h.  Surely there's a better way to tell it not to free the
pointer at this point in time other than this.

> > > +     xa_store(&auxdev->irqs, irq, no_free_ptr(info), GFP_KERNEL);
> > > +     return 0;
> > > +
> > > +sysfs_add_err:
> > > +     xa_erase(&auxdev->irqs, irq);
> > > +     return ret;
> > > +}
> > > +EXPORT_SYMBOL_GPL(auxiliary_device_sysfs_irq_add);
> > > +
> > > +/**
> > > + * auxiliary_device_sysfs_irq_remove - remove a sysfs entry for the given IRQ
> > > + * @auxdev: auxiliary bus device to add the sysfs entry.
> > > + * @irq: the IRQ to remove.
> > > + *
> > > + * This function should be called to remove an IRQ sysfs entry.
> > > + * The driver must invoke this API when IRQ is released by the device.
> > > + */
> > > +void auxiliary_device_sysfs_irq_remove(struct auxiliary_device *auxdev, int irq)
> > > +{
> > > +     struct auxiliary_irq_info *info __free(kfree) = xa_load(&auxdev->irqs, irq);
> > 
> > No verification that this is an actual entry before you dereferenced it?
> > Bold move...
> 
> Driver must do this for allocated irq. So xa_load cannot fail.
> In previous versions we had WARN_ON to catch driver bugs, but you didn’t
> like it.

Yes, because if something can happen, you handle the error properly, you
don't reboot a machine.

> I think this is fine the way it is in v9.

No, you are now causing a NULL dereference (or close to it) if something
went wrong.  Properly check this and handle it correctly.

thanks,

greg k-h