linux-kernel - Re: [RFC PATCH v2] clk: Use a new helper in managed functions

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:   Thu, 23 Jan 2020 13:17:59 +0100
From:   Marc Gonzalez <marc.w.gonzalez@...e.fr>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
        Sudip Mukherjee <sudipm.mukherjee@...il.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        LKML <linux-kernel@...r.kernel.org>,
        Russell King <linux@...linux.org.uk>,
        Guenter Roeck <linux@...ck-us.net>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Robin Murphy <robin.murphy@....com>,
        linux-clk <linux-clk@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [RFC PATCH v2] clk: Use a new helper in managed functions

On 23/01/2020 11:32, Geert Uytterhoeven wrote:

> On Thu, Jan 23, 2020 at 11:13 AM Marc Gonzalez wrote:
> 
>> A limitation of devm_add_action is that it stores the void *data argument "as is".
>> Users cannot pass the address of a struct on the stack. devm_add() addresses that
>> specific use-case, while being a minimal wrapper around devres_alloc + devres_add.
>> (devm_add_action adds an extra level of indirection.)
> 
> I didn't mean the advantage of devm_add() over devm_add_action(),
> but the advantage of dr_release_t, which has a device pointer.

I'm confused...

	void *devres_alloc(dr_release_t release, size_t size, gfp_t gfp);
	int devm_add_action(struct device *dev, void (*action)(void *), void *data);

devres_alloc() expects a dr_release_t argument; devm_add() is a thin wrapper
around devres_alloc(); ergo devm_add() expects that dr_release_t argument.

devm_add_action() is a "heavier" wrapper around devres_alloc() which defines
a "private" release function which calls a user-defined "action".
(i.e. the extra level of indirection I mentioned above.)

I don't understand the question about the advantage of dr_release_t.

>>>> +       void *data = devres_alloc(func, size, GFP_KERNEL);
>>>> +
>>>> +       if (data) {
>>>> +               memcpy(data, arg, size);
>>>> +               devres_add(dev, data);
>>>> +       } else
>>>> +               func(dev, arg);
>>>> +
>>>> +       return data;
>>>
>>> Why return data or NULL, instead of 0 or -Efoo, like devm_add_action()?
>>
>> My intent is to make devm_add a minimal wrapper (it even started out as
>> a macro). As such, I just transparently pass the result of devres_alloc.
>>
>> Do you see an advantage in processing the result?
> 
> There are actually two questions to consider here:
>   1. Is there a use case for returning the data pointer?
>      I.e. will the caller ever use it?
>   2. Can there be another failure mode than out-of-memory?
>      Changing from NULL to ERR_PTR() later means that all callers
>      need to be updated.

I think I see your point. You're saying it's not good to kick the can down
the road, because callers won't know what to do with the pointer.

Actually, I'm in the same boat as these users. I looked at
devres_alloc -> devres_alloc_node -> alloc_dr -> kmalloc_node_track_caller -> __do_kmalloc

Basically, the result is NULL when something went wrong, but the actual
error condition is not propagated. It could be:
1) check_add_overflow() finds an overflow
2) size > KMALLOC_MAX_CACHE_SIZE
3) kmalloc_slab() or kasan_kmalloc() fail
4) different errors on the CONFIG_NUMA path

Basically, if lower-level functions don't propagate errors, it's not
easy for a wrapper to do something sensible... ENOMEM looks reasonable
for kmalloc-related failures.

Regards.