linux-kernel - Re: [PATCH net-next 4/6] devlink: Clean registration of devlink port

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: <YZYBhArHOAbLfOUb@unreal>
Date:   Thu, 18 Nov 2021 09:32:20 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Andrew Lunn <andrew@...n.ch>, Aya Levin <ayal@...lanox.com>,
        Claudiu Manoil <claudiu.manoil@....com>, drivers@...sando.io,
        Florian Fainelli <f.fainelli@...il.com>,
        Ido Schimmel <idosch@...dia.com>,
        intel-wired-lan@...ts.osuosl.org,
        Ioana Ciornei <ioana.ciornei@....com>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Jiri Pirko <jiri@...dia.com>, linux-kernel@...r.kernel.org,
        linux-rdma@...r.kernel.org,
        Michael Chan <michael.chan@...adcom.com>,
        netdev@...r.kernel.org, oss-drivers@...igine.com,
        Saeed Mahameed <saeedm@...dia.com>,
        Shannon Nelson <snelson@...sando.io>,
        Simon Horman <simon.horman@...igine.com>,
        Taras Chornyi <tchornyi@...vell.com>,
        Tariq Toukan <tariqt@...dia.com>,
        Tony Nguyen <anthony.l.nguyen@...el.com>,
        UNGLinuxDriver@...rochip.com,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <vladimir.oltean@....com>
Subject: Re: [PATCH net-next 4/6] devlink: Clean registration of devlink port

On Wed, Nov 17, 2021 at 08:49:29PM -0800, Jakub Kicinski wrote:
> On Wed, 17 Nov 2021 20:26:20 +0200 Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@...dia.com>
> > 
> > devlink_port_register() is in-kernel API and as such can't really fail
> > as long as driver author didn't make a mistake by providing already existing
> > port index. Instead of relying on various error prints from the driver,
> > convert the existence check to be WARN_ON(), so such a mistake will be
> > caught easier.
> > 
> > As an outcome of this conversion, it was made clear that this function
> > should be void and devlink->lock was intended to protect addition to
> > port_list.
> 
> Leave this error checking in please.

Are you referring to error checks in the drivers or the below section
from devlink_port_register()?

       mutex_lock(&devlink->lock);
       if (devlink_port_index_exists(devlink, port_index)) {
               mutex_unlock(&devlink->lock);
               return -EEXIST;
       }

Because if it is latter, any driver (I didn't find any) that will rely
on this -EEXIST field should have some sort of locking in top level.
Otherwise nothing will prevent from doing port unregister right
before "return --EXEEXIST".

So change to WARN_ON() will be much more effective in finding wrong
drivers, because they manage port_index and not devlink.

And because this function can't fail, the drivers have a plenty of dead
code.

Thanks