Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 18 Jan 2023 17:15:04 -0800
From:   Lucas De Marchi <lucas.demarchi@intel.com>
To:     Luis Chamberlain <mcgrof@kernel.org>
CC:     Petr Mladek <pmladek@suse.com>,
        Prarit Bhargava <prarit@redhat.com>,
        Vegard Nossum <vegard.nossum@oracle.com>,
        Borislav Petkov <bp@alien8.de>, NeilBrown <neilb@suse.de>,
        Goldwyn Rodrigues <rgoldwyn@suse.com>,
        Petr Pavlu <petr.pavlu@suse.com>, <david@redhat.com>,
        <mwilck@suse.com>, <linux-modules@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <stable@vger.kernel.org>
Subject: Re: [PATCH v2] module: Don't wait for GOING modules
Message-ID: <20230119011504.q23askbtbwzwmica@ldmartin-desk2.lan>
References: <20221205103557.18363-1-petr.pavlu@suse.com>
 <Y5gI/3crANzRv22J@bombadil.infradead.org>
 <Y5hRRnBGYaPby/RS@alley>
 <Y8c3hgVwKiVrKJM1@bombadil.infradead.org>
Content-Type: text/plain; charset="us-ascii"; format=flowed
Content-Disposition: inline
In-Reply-To: <Y8c3hgVwKiVrKJM1@bombadil.infradead.org>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?cTjgFOULhw7Z8n1RDfLNFS/74ZPYAgctGqRkD9YYEFoNbeFyLhOhPHF48/gD?=
 =?us-ascii?Q?ZTp9La5UYwa2U+hWlmXwApA5Ue/NLB0+IApjD4XumYr5od3xzl7/Yksd7fyv?=
 =?us-ascii?Q?WrShtB9GooDKDOavMplUg+ihszW59vf0SS+uz/Y+vYhv+l5XIOkWizeFu2F4?=
 =?us-ascii?Q?yjM5mHGncLBBENMftU4zBEnyRIrs1dfAPTHzlKlNSjkgxf29c93lDiqlZIYZ?=
 =?us-ascii?Q?l6I/owHUbBMGFVOIrirdfCG+ibz7kiLNVdTpuvY1YyWC5SSpJxWNnvSYe+qC?=
 =?us-ascii?Q?9y/phzG8XL9C6vxEu95RjutuLnd2QKXCPdheCbeXXTpLLdTVCYTQilckZ0mB?=
 =?us-ascii?Q?bJzOwdZcU+qwyQNNPezPxmHlfnGMqbR+o7s7UG7K5C5EQFAM3U+og8hRSTIP?=
 =?us-ascii?Q?ptDNWG0E59Akr/2uJjbCtqvYe7LHHLx/ibmEoH9LDVLvdnbm9qGY84JR5W1o?=
 =?us-ascii?Q?1qsABZ+s5hDfdeR4kq+YxG3bhii48irF7ST6hLCSkrlt05q77X1fov0MTbDI?=
 =?us-ascii?Q?MWJrLK7cl2XevtptRfkhe+iRPT7qwHTBI50DVl9XkkIzzfdYfaAGSAepFaQ9?=
 =?us-ascii?Q?kfSkZ6iqSzUQbBQTj0gdD5aSyLcD2jhttXTmS8APcBrwNqK1Wz4QFONNzDzF?=
 =?us-ascii?Q?h8Hhd3CQAkeDwCF+vGv25rIRxabvtDv+k6LL8DKzH65WbCTOgt8AV/x+/+k/?=
 =?us-ascii?Q?43bUfXgvZU9+/9WtFqQ2ec3WEPv1pYFtVEgaa3I43uyexoOTiEfPBweoANNw?=
 =?us-ascii?Q?Vn34SvKAHyuwDvX/UVqIBndvO3M6lSH1MH+Vny0Cwsu7i9yZ3aAxP6zkHSxL?=
 =?us-ascii?Q?8dQDmMp9IfKtrL8JGvE9SePhSAxZRIP154MOP8ryVy5CbLx3qjNlGnPCaFXD?=
 =?us-ascii?Q?ofDAxQ7OI1HfjefkdHAwbSIUkBIfyWjwGn2/5Fy0pJ61EP1Mt8iXzydjIoHY?=
 =?us-ascii?Q?GHXuVoW6k6VuwRd5fh+xCcXPDQh+m0jCSy+jBWcICRwmuVn/R9C82NkURPqY?=
 =?us-ascii?Q?NUGJn6pniSjVi3e2lkGIdb2+u3EQSam/il39NK7imiJ4nyAYX3L5W1p/NLhe?=
 =?us-ascii?Q?sPxGhqALVSDB3E3nI/sRtD4ArXw6vu3iOQ3NxK6RNgFKc2by6yf1jq4r0/Bf?=
 =?us-ascii?Q?oSUeY6zmcKVTCFXr1+LVHJrMvkLQlVK6+wi0k7o6JggwLxJEOcaSANNThRix?=
 =?us-ascii?Q?xj7I2jnry5eHUXeG8i4VNExgk3Hi7RzrOgJ/AoW3Luum2KMNEsdReITgB0Oj?=
 =?us-ascii?Q?Y2oC7krydx69dIz/0ABi6UUo0lEdM3xu7e/lY8Q+csqL/akdX2jfCjJz5OSc?=
 =?us-ascii?Q?VLdyTcJWoaOT8/GGIJ53rlOxnxxcVTklk7wx2ijoFGbCxkB0PRcDjxeln20C?=
 =?us-ascii?Q?/3kl42HG4cWcqMD/r+4/FRMiYB6xUBCAytBE7BEeLjc6lAlV89bd6Y+neQvq?=
 =?us-ascii?Q?DmQX2dSwMxaWflUG3yZZtGnHsckEk1HOIPTjqlic7E6jqAiu5jaBu3M1T8k6?=
 =?us-ascii?Q?N6uPZgWQfF98aqBAoWuQLCNoho93lX1jWkvvHn25/7lH+GS9pe26WWqRa0Gf?=
 =?us-ascii?Q?v7GV01TGr+27vc/AzvpnjKfo5ExoCzN+IgpBnF1eOZFbpMgph5LHVrXWEZai?=
 =?us-ascii?Q?xg=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9a00358b-7543-4f39-3f44-08daf9ba99ed
X-MS-Exchange-CrossTenant-AuthSource: CY5PR11MB6139.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Jan 2023 01:15:07.4336
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: YdKpcwpBks35TDa+lQ5lZ+ewTFsXC6ypS9k280CBr49Ov4USWX6d6FJf0tWFI0q3XOoQ3hkX9ZC0/TruOzB44CbDaviydJuN5T1CLlRqD5c=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB6025
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Jan 17, 2023 at 04:04:22PM -0800, Luis Chamberlain wrote:
>On Tue, Dec 13, 2022 at 11:17:42AM +0100, Petr Mladek wrote:
>> On Mon 2022-12-12 21:09:19, Luis Chamberlain wrote:
>> > On Mon, Dec 05, 2022 at 11:35:57AM +0100, Petr Pavlu wrote:
>> > > diff --git a/kernel/module/main.c b/kernel/module/main.c
>> > > index d02d39c7174e..7a627345d4fd 100644
>> > > --- a/kernel/module/main.c
>> > > +++ b/kernel/module/main.c
>> > > @@ -2386,7 +2386,8 @@ static bool finished_loading(const char *name)
>> > >  	sched_annotate_sleep();
>> > >  	mutex_lock(&module_mutex);
>> > >  	mod = find_module_all(name, strlen(name), true);
>> > > -	ret = !mod || mod->state == MODULE_STATE_LIVE;
>> > > +	ret = !mod || mod->state == MODULE_STATE_LIVE
>> > > +		|| mod->state == MODULE_STATE_GOING;
>> > >  	mutex_unlock(&module_mutex);
>> > >
>> > >  	return ret;
>> > > @@ -2562,20 +2563,35 @@ static int add_unformed_module(struct module *mod)
>> > >
>> > >  	mod->state = MODULE_STATE_UNFORMED;
>> > >
>> > > -again:
>> >
>> > So this is part of my biggest concern for regression, the removal of
>> > this tag and its use.
>> >
>> > Before this we always looped back to trying again and again.
>>
>> Just to be sure that we are on the same page.
>>
>> The loop was _not_ infinite. It serialized all attempts to load
>> the same module. In our case, it serialized all failures and
>> prolonged the pain.
>
>That's fair yes. The loop happens so long as an already existing module is
>present with the same name.
>
>> > >  	mutex_lock(&module_mutex);
>> > >  	old = find_module_all(mod->name, strlen(mod->name), true);
>> > >  	if (old != NULL) {
>> > > -		if (old->state != MODULE_STATE_LIVE) {
>> > > +		if (old->state == MODULE_STATE_COMING
>> > > +		    || old->state == MODULE_STATE_UNFORMED) {
>> > >  			/* Wait in case it fails to load. */
>> > >  			mutex_unlock(&module_mutex);
>> > >  			err = wait_event_interruptible(module_wq,
>> > >  					       finished_loading(mod->name));
>> > >  			if (err)
>> > >  				goto out_unlocked;
>> > > -			goto again;
>> >
>> > We essentially bound this now, and before we didn't.
>> >
>> > Yes we we wait for finished_loading() of the module -- but if udev is
>> > hammering tons of same requests, well, we *will* surely hit this, as
>> > many requests managed to get in before userspace saw the module present.
>> >
>> > While this typically can be useful, it means *quite a bit* of conditions which
>> > definitely *did* happen before will now *bail out* fast, to the extent
>> > that I'm not even sure why we just re-try once now.
>>
>> I do not understand this. We do _not_ re-try the load in the new
>> version. We just wait for the result of the parallel attempt to
>> load the module.
>>
>> Maybe, you are confused that we repeat find_module_all(). But it is
>> the way how to find the result of the parallel load.
>
>My point is that prior to the buggy commit 6e6de3dee51a ("kernel/module.c: Only
>return -EEXIST for modules that have finished loading") and even after that
>commit it we 'goto again' if an old request is found. We now simply bound this
>right away. Yes, the loop was not infinite, but in theory at least a few
>iterations were possible before whereas now immediately return -EBUSY
>and I don't think all use cases may be ready yet.
>
>> > If we're going to
>> > just re-check *once* why not do something graceful like *at least*
>> > cond_resched() to let the system breathe for a *tiny bit*.
>>
>> We must check the result under module_mutex. We have to take this
>> sleeping lock. There is actually a rescheduling. I do not think that
>> cond_resched() would do any difference.
>
>Makes sense.
>
>> > > +
>> > > +			/* The module might have gone in the meantime. */
>> > > +			mutex_lock(&module_mutex);
>> > > +			old = find_module_all(mod->name, strlen(mod->name),
>> > > +					      true);
>> > >  		}
>> > > -		err = -EEXIST;
>> > > +
>> > > +		/*
>> > > +		 * 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;
>> >
>> > And for all those use cases we end up here now, with -EBUSY. So udev
>> > before was not bounded, and kept busy-looping on the retry in-kernel,
>> > and we now immediately bound its condition to just 2 tries to see if the
>> > old module existed and now *return* a new value to userspace.
>> >
>> > My main concerns are:
>> >
>> > 0) Why not use cond_resched() if we're just going to check twice?
>>
>> We take module_mutex. It should cause even bigger delay than cond_resched().
>
>ACK.
>
>> > 1) How are we sure we are not regressing userspace by removing the boundless
>> > loop there? (even if the endless loop was stupid)
>>
>> We could not be sure. On the other hand, if more attempts help to load
>> the module then it is racy and not reliable. The new approach would
>> make it better reproducible and fix the race.
>
>Yes, but the short cut it is a userspace visible change.
>
>> > 2) How is it we expect that we won't resgress userspace now by bounding
>> > that check and pretty much returning -EBUSY right away? This last part
>> > seems dangerous, in that if userspace did not expect -EBUSY and if an
>> > error before caused a module to fail and fail boot, why wouldn't we fail
>> > boot now by bailing out faster??
>>
>> Same answer as for 1)
>>
>>
>> > 3) *Fixing* a kernel regression by adding new expected API for testing
>> > against -EBUSY seems not ideal.
>>
>> IMHO, the right solution is to fix the subsystems so that they send
>> only one uevent.
>
>Makes sense, but that can take time and some folks are stuck on old kernels
>and perhaps porting fixes for this on subsystems may take time to land
>to some enterprisy kernels. And then there is also systemd that issues
>the requests too, at least that was reflected in commit 6e6de3dee51a
>("kernel/module.c: Only return -EEXIST for modules that have finished loading")
>that commit claims it was systemd issueing the requests which I mean to
>interpret finit_module(), not calling modprobe.

just a comment on this, if it helps anything: commit 6e6de3dee51a says
systemd, but it would be more accurate to say udev or systemd-udev to
disambiguate with pid 1.

udev uses libkmod to load modules so it's pretty much the same logic as
modprobe.

Lucas De Marchi