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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 5 Jul 2024 08:35:33 +0300
From: Shay Drori <shayd@...dia.com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: <netdev@...r.kernel.org>, <pabeni@...hat.com>, <davem@...emloft.net>,
	<kuba@...nel.org>, <edumazet@...gle.com>, <david.m.ertman@...el.com>,
	<rafael@...nel.org>, <ira.weiny@...el.com>, <linux-rdma@...r.kernel.org>,
	<leon@...nel.org>, <tariqt@...dia.com>, Simon Horman <horms@...nel.org>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>, Parav Pandit
	<parav@...dia.com>
Subject: Re: [PATCH net-next v9 1/2] driver core: auxiliary bus: show
 auxiliary device IRQs



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?


> 
> 
>> +     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.
I think this is fine the way it is in v9.

> 
> greg k-h