netdev - Re: [PATCH net-next] netlink: provide an ability to set default extack message

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]

Message-ID: <Y9NgdXk3NLtjG3Mj@unreal>
Date:   Fri, 27 Jan 2023 07:26:13 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Andrew Lunn <andrew@...n.ch>,
        bridge@...ts.linux-foundation.org,
        Eric Dumazet <edumazet@...gle.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        netdev@...r.kernel.org, Nikolay Aleksandrov <razor@...ckwall.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Roopa Prabhu <roopa@...dia.com>
Subject: Re: [PATCH net-next] netlink: provide an ability to set default
 extack message

On Thu, Jan 26, 2023 at 02:37:23PM -0800, Jakub Kicinski wrote:
> On Fri, 27 Jan 2023 00:32:13 +0200 Vladimir Oltean wrote:
> > On Thu, Jan 26, 2023 at 09:15:03PM +0200, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro@...dia.com>
> > > 
> > > In netdev common pattern, xxtack pointer is forwarded to the drivers  
> >                             ~~~~~~
> >                             extack
> > 
> > > to be filled with error message. However, the caller can easily
> > > overwrite the filled message.
> > > 
> > > Instead of adding multiple "if (!extack->_msg)" checks before any
> > > NL_SET_ERR_MSG() call, which appears after call to the driver, let's
> > > add this check to common code.
> > > 
> > > [1] https://lore.kernel.org/all/Y9Irgrgf3uxOjwUm@unreal
> > > Signed-off-by: Leon Romanovsky <leonro@...dia.com>
> > > ---  
> > 
> > I would somewhat prefer not doing this, and instead introducing a new
> > NL_SET_ERR_MSG_WEAK() of sorts.
> 
> That'd be my preference too, FWIW. It's only the offload cases which
> need this sort of fallback.

Of course not, almost any error unwind path which sets extack will need it.
See devlink as an example, but I'm confident that same issue exists in other
places too.

You are suggesting API which is very easy to do wrong.

So I prefer to stay with my proposal if it is possible.

Thanks