linux-kernel - Re: [PATCH] soundwire: intel: move to auxiliary bus

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:   Wed, 24 Mar 2021 09:55:01 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     alsa-devel@...a-project.org, vinod.koul@...aro.org,
        linux-kernel@...r.kernel.org, hui.wang@...onical.com,
        vkoul@...nel.org, srinivas.kandagatla@...aro.org,
        sanyog.r.kale@...el.com,
        Bard Liao <yung-chuan.liao@...ux.intel.com>,
        rander.wang@...ux.intel.com, bard.liao@...el.com
Subject: Re: [PATCH] soundwire: intel: move to auxiliary bus


>>>>>> Note that the auxiliary bus API has separate init and add steps, which
>>>>>> requires more attention in the error unwinding paths. The main loop
>>>>>> needs to deal with kfree() and auxiliary_device_uninit() for the
>>>>>> current iteration before jumping to the common label which releases
>>>>>> everything allocated in prior iterations.
>>>>>
>>>>> The init/add steps can be moved together in the aux bus code if that
>>>>> makes this usage simpler.  Please do that instead.
>>>>
>>>> IIRC the two steps were separated during the auxbus reviews to allow the
>>>> parent to call kfree() on an init failure, and auxiliary_device_uninit()
>>>> afterwards.
>>>>
>>>> https://www.kernel.org/doc/html/latest/driver-api/auxiliary_bus.html#auxiliary-device
>>>>
>>>> With a single auxbus_register(), the parent wouldn't know whether to use
>>>> kfree() or auxiliary_device_uinit() when an error is returned, would it?
>>>>
>>>
>>> It should, you know the difference when you call device_register() vs.
>>> device_initialize()/device_add(), for what to do, right?
>>>
>>> Should be no difference here either :)
>>
>> sorry, not following.
>>
>> with the regular devices, the errors can only happen on the second "add"
>> stage.
>>
>> int device_register(struct device *dev)
>> {
>> 	device_initialize(dev);
>> 	return device_add(dev);
>> }
>>
>> that's not what is currently implemented for the auxiliary bus
>>
>> the current flow is
>>
>> ldev = kzalloc(..)
>> some inits
>> ret = auxiliary_device_init(&ldev->auxdev)
>> if (ret < 0) {
>>      kfree(ldev);
>>      goto err1;
>> }
>>
>> ret = auxiliary_device_add(&ldev->auxdev)
>> if (ret < 0)
>>      auxiliary_device_uninit(&ldev->auxdev)
>>      goto err2;
>> }
>> ...
>> err2:
>> err1:
>>
>> How would I convert this to
>>
>> ldev = kzalloc(..)
>> some inits
>> ret = auxiliary_device_register()
>> if (ret) {
>>     kfree(ldev) or not?
>>     unit or not?
>> }
>>
>> IIRC during reviews there was an ask that the parent and name be checked,
>> and that's why the code added the two checks below:
>>
>> int auxiliary_device_init(struct auxiliary_device *auxdev)
>> {
>> 	struct device *dev = &auxdev->dev;
>>
>> 	if (!dev->parent) {
>> 		pr_err("auxiliary_device has a NULL dev->parent\n");
>> 		return -EINVAL;
>> 	}
>>
>> 	if (!auxdev->name) {
>> 		pr_err("auxiliary_device has a NULL name\n");
>> 		return -EINVAL;
>> 	}
>>
>> 	dev->bus = &auxiliary_bus_type;
>> 	device_initialize(&auxdev->dev);
>> 	return 0;
>> }
>>
>> does this clarify the sequence?
> 
> Yes, thanks, but I don't know the answer to your question, sorry.  This
> feels more complex than it should be, but I do not have the time at the
> moment to look into it, sorry.
> 
> Try getting the authors of this code to fix it up :)

We can try to check why those two tests were added before initialize(), 
I don't fully recall these details

If we could move these tests after device_initialize() then we could add 
a _register function.

Note at this point it would mean an API change and impact the existing 
Nvidia/Mellanox code, we are using the same sequence as them

https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/mellanox/mlx5/core/dev.c#L262