From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A2E41C31E4C for ; Fri, 14 Jun 2019 16:37:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 853822183E for ; Fri, 14 Jun 2019 16:37:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726340AbfFNQg5 (ORCPT ); Fri, 14 Jun 2019 12:36:57 -0400 Received: from mx2.suse.de ([195.135.220.15]:38268 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726063AbfFNQg5 (ORCPT ); Fri, 14 Jun 2019 12:36:57 -0400 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id E210FAF58; Fri, 14 Jun 2019 16:36:55 +0000 (UTC) Date: Fri, 14 Jun 2019 18:36:55 +0200 From: Libor Pechacek To: Joe Lawrence Cc: Petr Mladek , Miroslav Benes , linux-kernel@vger.kernel.org, live-patching@vger.kernel.org, linux-kbuild@vger.kernel.org Subject: Re: [PATCH v4 00/10] klp-convert livepatch build tooling Message-ID: <20190614163655.GC15002@fm.suse.cz> References: <20190509143859.9050-1-joe.lawrence@redhat.com> <20190614083435.uq3mk6mprbatysol@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri 14-06-19 10:20:09, Joe Lawrence wrote: > On 6/14/19 4:34 AM, Petr Mladek wrote: [...] > > Anyway, I am curious about one thing. I saw: > > > > function __load_mod() { > > local mod="$1"; shift > > > > local msg="% modprobe $mod $*" > > log "${msg%% }" > > ret=$(modprobe "$mod" "$@" 2>&1) > > if [[ "$ret" != "" ]]; then > > die "$ret" > > fi > > > > # Wait for module in sysfs ... > > loop_until '[[ -e "/sys/module/$mod" ]]' || > > die "failed to load module $mod" > > } > > > > Is the waiting for sysfs really necessary here? > > > > Note that it is /sys/module and not /sys/kernel/livepatch/. > > I can't remember if that was just paranoid-protective-bash coding or > actually required. Libor provided great feedback on the initial patch > series that introduced the self-tests, perhaps he remembers. I don't recall analyzing this spot in detail but looking at it now I don't see anything wrong with it. While the check is likely superfluous, I'm not against keeping it in place. > > My understanding is that modprobe waits until the module succesfully > > loaded. mod_sysfs_setup() is called before the module init callback. > > Therefore the sysfs interface should be read before modprobe returns. > > Do I miss something? > > > > If it works different way then there might be some races because > > mod_sysfs_setup() is called before the module is alive. > > All of this is called from a single bash script function, so in a call stack > fashion, something like this would occur when loading a livepatch module: > > [ mod_sysfs_setup() ] > modprobe waits for: .init complete, MODULE_STATE_LIVE > __load_mod() waits for: /sys/module/$mod > load_lp_nowait() waits for: /sys/kernel/livepatch/$mod > load_lp() waits for: /sys/kernel/livepatch/$mod/transition = 0 > test-script.sh > > So I would think that by calling modprobe, we ensure that the module code is > ready to go. The /sys/module/$mod check might be redundant as you say, but > because modprobe completed, we should be safe, no? > > The only "nowait" function we have is load_lp_nowait(), which would let us > march onward before the livepatch transition may have completed. And even that one is waiting for the live patch module name appear under /sys/kernel/livepatch/. This is IMHO acceptable level of paranoia. Libor -- Libor Pechacek SUSE Labs