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

Date:   Mon, 11 Dec 2023 20:13:00 +0530
From:   Pintu Agarwal <pintu.ping@...il.com>
To:     Rob Herring <robh@...nel.org>, vichy.kuo@...il.com
Cc:     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

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 ?


Thanks,
Pintu