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 Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 63CD3C433FE for ; Fri, 30 Sep 2022 06:41:56 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 75DD984C50; Fri, 30 Sep 2022 08:41:53 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="WtD/QECw"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 17B9E84ABC; Fri, 30 Sep 2022 08:41:51 +0200 (CEST) Received: from mail-pj1-x1032.google.com (mail-pj1-x1032.google.com [IPv6:2607:f8b0:4864:20::1032]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 1A2018317B for ; Fri, 30 Sep 2022 08:41:48 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=takahiro.akashi@linaro.org Received: by mail-pj1-x1032.google.com with SMTP id h8-20020a17090a054800b00205ccbae31eso8089471pjf.5 for ; Thu, 29 Sep 2022 23:41:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc :subject:date:message-id:reply-to; bh=i5iAMY7WII37YYTiiP+dIFQEbrqdIWIypqrcbuY0t1Q=; b=WtD/QECwv5EDXaWK+PL7GBAlosu2hS5qWbo3+bh0djmLhQJwJQl9XK9zyIQJLGnFLO bGHMcn4wmy1vWR3selJpXTHgMUJ144b/z++88/9EunphkDyT1ivcXbYCxiQg8rn7c10G 8iiq6GQWf2fESpj/oKeYKOpZRZut0COJtogbO9EV0IN7GQGVMXcTQZroBtncszx2YTiX hGo3QSqLwgAGerzPGN7HJg3ET0hxvAqSOISL5P+b23Ca+c56uMSibYBW30xXMg1/xJae r2LN3VHv8yL06jV1S/JFahnYv3iV+1oCfTTtsQubI79vrI5ONdXlWl4myJ1jFSspipuN 7NYQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=i5iAMY7WII37YYTiiP+dIFQEbrqdIWIypqrcbuY0t1Q=; b=PQPF0SN4tYdYBUv7yLu4YE3gu1oSO2m+2b7h+orQm5vMAn9wBhhIS2R4BDws+PtyKY dJ1khZU7mEByouZkHM/9yoXd4BZBTjfvSePVOoGg7NrXzdBinzfhIMHfirNvevhh+pg+ Iy7PjrpkWdJZ4qbpbH89YUlEnL/37vvx773jAiusTdd9kHzKNCApwadw9o5J3lUhvSHe xOG0HPdaV9ReFQAiq/bF1KQhD/KtSS+oLYFBuk7r6IughHmAJVqkjzQp1s/k/uitu2fN vOMc8gLqQiU7mws9rmL0++WwoWk58yOguka/Hx9cVu4BAm9g8aabo8VRHO7J5QEF9ScA ksqg== X-Gm-Message-State: ACrzQf3iZhGB8DIV+ToKk1+JR/vocmFi1XEfQyFefu1O5UsmDJw0HJUk QXMM3AvcZ5NahGmHV20GHt6syA== X-Google-Smtp-Source: AMsMyM4jlT56WGYa3jl6MppAUXMa9997i96Q5qYXQuOXQSek6d3AU2L6jjZRhvo+CX1uqDB3Pzfrzg== X-Received: by 2002:a17:902:bf46:b0:179:eba5:90ba with SMTP id u6-20020a170902bf4600b00179eba590bamr7375317pls.16.1664520106041; Thu, 29 Sep 2022 23:41:46 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:53a5:1102:193e:47f0]) by smtp.gmail.com with ESMTPSA id k12-20020a170902ce0c00b00176e6f553efsm1053676plg.84.2022.09.29.23.41.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 29 Sep 2022 23:41:45 -0700 (PDT) Date: Fri, 30 Sep 2022 15:41:42 +0900 From: AKASHI Takahiro To: Ilias Apalodimas Cc: Heinrich Schuchardt , u-boot@lists.denx.de Subject: Re: [PATCH 1/1] efi_loader: fix efi_initrd_deregister() Message-ID: <20220930064142.GA17468@laputa> Mail-Followup-To: AKASHI Takahiro , Ilias Apalodimas , Heinrich Schuchardt , u-boot@lists.denx.de References: <20220929235748.25608-1-heinrich.schuchardt@canonical.com> <20220930014731.GA10018@laputa> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean Ilias, On Fri, Sep 30, 2022 at 09:18:35AM +0300, Ilias Apalodimas wrote: > Akashi-san > > On Fri, 30 Sept 2022 at 04:47, AKASHI Takahiro > wrote: > > > > On Fri, Sep 30, 2022 at 01:57:48AM +0200, Heinrich Schuchardt wrote: > > > Don't try to delete a non-existent handle. > > > > It is okay as a safe guard, but it doesn't fix underlying issues. > > I dont think we safeguard anything. That code path won't try to delete > anything regardless? > > > > > efi_initrd_register() is called only in efi_bootmgr_load(), and so > > efi_initrd_deregister() should be called only at the paired location. > > There's a reason for that. > > > > - Remove efi_initrd_deregister() from do_bootefi_exec() > > - do_efibootmgr() should look like > > efi_bootmgr_load() > > do_bootefi_exec() > > efi_initrd_deregister() > > Otherwise, do_bootefi_exec() always tries to free a handle in > > the other cases (i.e. bootefi ). > > > > Another issue is there in try_load_entry() called by efi_bootmgr_load(). > > (after efi_initrd_register()) > > if (size) { > > *load_options = malloc(size); > > if (!*load_options) { > > ret = EFI_OUT_OF_RESOURCES; > > goto error; > > } > > ... > > > > If malloc() fails, we should call efi_initrd_deregister() within > > try_load_entry(). > > > > Should I submit a patch? > > The whole implementation on the *kernel* assumes the protocol is > present if the file it's pointing is real and existing. You also need > to have a single instance of the protocol installed. IOW if you > install the protocol and the initrd is not there, the kernel won't > fallback on the dt /chosen/ node or the initrd= in the cmdline. Yes, I confirmed that before I made my comment. > The > whole initrd loading logic depends on BootCurrnent, which iirc is not > set yet on the flow you are proposing. I don't get your point. In do_efibootmgr(), what I suggested above is: - efi_bootmgr_load() installs LOAD_FILE2_PROTOCOL if initrd file exists, and if this function fails, LOAD_FILE2_PROTOCOL must be uninstalled any way. - after returning from UEFI app invoked by do_bootefi_exec(), - we should simply uninstall LOAD_FILE2_RPTOCOL by calling efi_initrd_deregister(). In "bootefi " case, efi_bootmgr_load() is not called, so LOAD_FILE2_PROTOCOL won't be installed for loading initrd file. Why do we have to call efi_initrd_deregister() in that case? Regarding BootCurrent, I don't think it has nothing to do with the discussion above. Anyhow it *is* set before reaching "if (size) ...". -Takahiro Akashi > Regards > /Ilias > > > > -Takahiro Akashi > > > > > Signed-off-by: Heinrich Schuchardt > > > --- > > > lib/efi_loader/efi_load_initrd.c | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c > > > index c5e6652e66..3d6044f760 100644 > > > --- a/lib/efi_loader/efi_load_initrd.c > > > +++ b/lib/efi_loader/efi_load_initrd.c > > > @@ -230,6 +230,9 @@ efi_status_t efi_initrd_register(void) > > > */ > > > void efi_initrd_deregister(void) > > > { > > > + if (!efi_initrd_handle) > > > + return; > > > + > > > efi_delete_handle(efi_initrd_handle); > > > efi_initrd_handle = NULL; > > > } > > > -- > > > 2.37.2 > > >