linux-kernel - Re: [PATCH] of: reserved_mem: fix error log for reserved mem init failure

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] [day] [month] [year] [list]

Message-ID: <CAOuPNLhc-U8YzkUAE2aDPUKAhzgXtTTWUC8faU=a62na2gdF_g@mail.gmail.com>
Date: Wed, 17 Jan 2024 22:55:31 +0530
From: Pintu Agarwal <pintu.ping@...il.com>
To: Rob Herring <robh@...nel.org>
Cc: vichy.kuo@...il.com, Pintu Kumar <quic_pintu@...cinc.com>, 
	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org, linux-mm@...ck.org, 
	frowand.list@...il.com, devicetree@...r.kernel.org
Subject: Re: [PATCH] of: reserved_mem: fix error log for reserved mem init failure

On Tue, 16 Jan 2024 at 20:05, Rob Herring <robh@...nel.org> wrote:
>
> On Sat, Jan 06, 2024 at 11:31:12PM +0530, Pintu Agarwal wrote:
> > Hi,
> >
> > On Thu, 14 Dec 2023 at 22:47, Pintu Agarwal <pintu.ping@...il.com> wrote:
> > >
> > > On Mon, 11 Dec 2023 at 20:13, Pintu Agarwal <pintu.ping@...il.com> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Sat, 9 Dec 2023 at 02:01, Rob Herring <robh@...nel.org> wrote:
> > > > >
> > > > > On Wed, Dec 06, 2023 at 08:46:00PM +0530, Pintu Kumar wrote:
> > > > > > During fdt_init_reserved_mem() when __reserved_mem_init_node()
> > > > > > fail we are using pr_info to print error.
> > > > > >
> > > > > > So, if we change the loglevel to 4 (or below), this error
> > > > > > message will be missed.
> > > > > >
> > > > > > Thus, change the pr_info to pr_err for fail case.
> > > > > >
> > > > > > Signed-off-by: Pintu Kumar <quic_pintu@...cinc.com>
> > > > > > ---
> > > > > >  drivers/of/of_reserved_mem.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c
> > > > > > index 7ec94cfcbddb..473665e76b6f 100644
> > > > > > --- a/drivers/of/of_reserved_mem.c
> > > > > > +++ b/drivers/of/of_reserved_mem.c
> > > > > > @@ -334,7 +334,7 @@ void __init fdt_init_reserved_mem(void)
> > > > > >               if (err == 0) {
> > > > > >                       err = __reserved_mem_init_node(rmem);
> > > > > >                       if (err != 0 && err != -ENOENT) {
> > > > > > -                             pr_info("node %s compatible matching fail\n",
> > > > > > +                             pr_err("node %s compatible matching fail\n",
> > > > >
> > > > > Isn't the message just wrong. If compatible match fails, we return
> > > > > ENOENT. The failure here would be from the init function.
> > > > >
> > > > Okay.
> > > > You mean to say, if __reserved_mem_init_node fails with default err
> > > > (ENOENT) then it may not hit this condition.
> > > > Instead it will hit the 'else' case which is wrong ?
> > > > Also, the "initfn" inside "__reserved_mem_init_node" may fail in which
> > > > case also it may return default err.
> > > >
> > > > Maybe, the initial author's intention was to free the memory only if
> > > > the failure type is not the default ENOENT type.
> > > >
> > > > This seems to be a different issue.
> > > > Can we address this separately in a different patch ?
> > > >
> > > > And how do we fix this ?
> > > > One option is to add another "if" condition with just ENOENT error check ?
> > > > if (err == -ENOENT) {
> > > >     pr_err("node %s compatible matching fail\n", rmem->name);
> > > >     return;
> > > > }
> > > > Then, correct the existing log with a different message:
> > > > pr_err("node %s matching reserved mem not found.\n", rmem->name);
> > > > Or, add one more "if else" condition ?
> > > > Or, fix the calling function itself : __reserved_mem_init_node ?
> > > >
> > >
> > > Any further comments on this ?
> >
> > Any further comments or suggestions on the above ?
> > Shall we just fix the log message, or correct the if/else case as well ?
>
> It looked to me like the original author's intent was this is not an
> error. Either convince me otherwise or wait for me to study this
> further. This code gets a lot of drive-by patches and what is "correct"
> isn't always clear.
>
Thank you so much for looking into this.
Yes, I agree that the author's intention might be to present this as
an info message to the user.
But I think he might have not realised that info msg may not be printed always.
If we change loglevel to 4 (quiet) this info msg may be missed.
Normally, in the final production system we do this.
So, ideally, I think, error msg logging is more suitable in either case.

Sure, I will wait for your further findings.
I will also have a further look and see if I can convince you more
with another set :)

Thanks,
Pintu