linux-kernel - Re: [PATCH 2/2] module: add support to avoid duplicates early on load

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]

Message-ID: <atfuud5a4zcrs5s6rh7t7eauuofhtwczvzd7r5tsg3ramuqrfq@sxbvdnhaaxgr>
Date:   Tue, 30 May 2023 22:30:11 -0700
From:   Lucas De Marchi <lucas.demarchi@...el.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
CC:     Luis Chamberlain <mcgrof@...nel.org>,
        Johan Hovold <johan@...nel.org>,
        Petr Pavlu <petr.pavlu@...e.com>, <gregkh@...uxfoundation.org>,
        <rafael@...nel.org>, <song@...nel.org>,
        <lucas.de.marchi@...il.com>, <christophe.leroy@...roup.eu>,
        <peterz@...radead.org>, <rppt@...nel.org>, <dave@...olabs.net>,
        <willy@...radead.org>, <vbabka@...e.cz>, <mhocko@...e.com>,
        <dave.hansen@...ux.intel.com>, <colin.i.king@...il.com>,
        <jim.cromie@...il.com>, <catalin.marinas@....com>,
        <jbaron@...mai.com>, <rick.p.edgecombe@...el.com>,
        <yujie.liu@...el.com>, <david@...hat.com>, <tglx@...utronix.de>,
        <hch@....de>, <patches@...ts.linux.dev>,
        <linux-modules@...r.kernel.org>, <linux-mm@...ck.org>,
        <linux-kernel@...r.kernel.org>, <pmladek@...e.com>,
        <prarit@...hat.com>, <lennart@...ttering.net>
Subject: Re: [PATCH 2/2] module: add support to avoid duplicates early on load

On Tue, May 30, 2023 at 06:17:11PM -0400, Linus Torvalds wrote:
>On Tue, May 30, 2023 at 3:41 PM Luis Chamberlain <mcgrof@...nel.org> wrote:
>>
>> OK thanks! So just to confirm, it seems fine to return the same error
>> code if duplicates wait, or do you prefer for some reason for there to
>> be an exception and return -EEXIST if the module did succeed in the
>> duplicate case?
>
>I think either should be fine, since either was possible before.
>
>By definition, these are module loads being done in parallel, and so
>any of them "could" have been the first, and returned success before.
>
>And by extension, any of them could have been not first, and returned
>-EEXIST if somebody else loaded the same module first.
>
>So that "somebody else did a load" code:
>
>        if (idempotent(&idem, file_inode(f))) {
>                wait_for_completion(&idem.complete);
>                return idem.ret;
>        }
>
>could certainly have made the return value be something like
>
>        return idem.ret ? : -EEXIST;

yes, this is what I had in mind.

>
>instead of that "return idem.ret".
>
>But it does seem simpler - and more in line with the conceptual
>"loading the same module is an idempotent operation" of the patch -
>to just always return the success value to all of them.
>
>After all, they all did in some sense succeed to get that module
>loaded, even if it was a communal effort, and some threads did more
>than others...
>
>As mentioned, I don't think it can matter either way, since any of the
>callers might as well have been the successful one, and they would
>basically have to act the same way regardless (ie "somebody else
>succeeded" and "you succeeded" are basically equivalent return

agreed, it will just be a slightly different behavior if finit_module()
is called twice and the first call is already in the process of
initializing the module, i.e. complete_formation() was already called,
putting the module in the MODULE_STATE_COMING state, as per
kernel/module/main.c:add_unformed_module():

	/*
	 * We are here only when the same module was being loaded. Do
	 * not try to load it again right now. It prevents long delays
	 * caused by serialized module load failures. It might happen
	 * when more devices of the same type trigger load of
	 * a particular module.
	 */
	if (old && old->state == MODULE_STATE_LIVE)
		err = -EEXIST;
	else
		err = -EBUSY;
	goto out;

in userspace we already deal with that in a special way and should be
compatible with returning 0 for all practical purposes.

thanks
Lucas De Marchi

>values). If the module was a prerequisite for another module being
>loaded, either -EEXIST or 0 _is_ a success case.
>
>             Linus