netdev - Re: [patch net-next 1/8] devlink: call devlink_port_register/unregister() on registered instance

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: <5e97d5b5-3df4-c9b5-bca4-c82c75d353e8@amd.com>
Date:   Mon, 5 Dec 2022 15:55:32 -0800
From:   Shannon Nelson <shnelson@....com>
To:     Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org
Cc:     davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
        edumazet@...gle.com, michael.chan@...adcom.com,
        ioana.ciornei@....com, dmichail@...gible.com,
        jesse.brandeburg@...el.com, anthony.l.nguyen@...el.com,
        tchornyi@...vell.com, tariqt@...dia.com, saeedm@...dia.com,
        leon@...nel.org, idosch@...dia.com, petrm@...dia.com,
        vladimir.oltean@....com, claudiu.manoil@....com,
        alexandre.belloni@...tlin.com, simon.horman@...igine.com,
        shannon.nelson@....com, brett.creeley@....com
Subject: Re: [patch net-next 1/8] devlink: call
 devlink_port_register/unregister() on registered instance

On 12/5/22 7:22 AM, Jiri Pirko wrote:
> 
> From: Jiri Pirko <jiri@...dia.com>
> 
> Change the drivers that use devlink_port_register/unregister() to call
> these functions only in case devlink is registered.
> 
> Signed-off-by: Jiri Pirko <jiri@...dia.com>
> ---
> RFC->v1:
> - shortened patch subject
> ---
>   .../net/ethernet/broadcom/bnxt/bnxt_devlink.c | 29 ++++++++++---------
>   .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  |  7 +++--
>   .../ethernet/fungible/funeth/funeth_main.c    | 17 +++++++----
>   drivers/net/ethernet/intel/ice/ice_main.c     | 21 ++++++++------
>   .../ethernet/marvell/prestera/prestera_main.c |  6 ++--
>   drivers/net/ethernet/mscc/ocelot_vsc7514.c    | 10 +++----
>   .../ethernet/pensando/ionic/ionic_devlink.c   |  6 ++--
>   drivers/net/ethernet/ti/am65-cpsw-nuss.c      |  7 +++--
>   8 files changed, 60 insertions(+), 43 deletions(-)
> 



> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> index e6ff757895ab..06670343f90b 100644
> --- a/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_devlink.c
> @@ -78,16 +78,18 @@ int ionic_devlink_register(struct ionic *ionic)
>          struct devlink_port_attrs attrs = {};
>          int err;
> 
> +       devlink_register(dl);
> +
>          attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL;
>          devlink_port_attrs_set(&ionic->dl_port, &attrs);
>          err = devlink_port_register(dl, &ionic->dl_port, 0);
>          if (err) {
>                  dev_err(ionic->dev, "devlink_port_register failed: %d\n", err);
> +               devlink_unregister(dl);
>                  return err;
>          }
> 
>          SET_NETDEV_DEVLINK_PORT(ionic->lif->netdev, &ionic->dl_port);
> -       devlink_register(dl);
>          return 0;
>   }
> 
> @@ -95,6 +97,6 @@ void ionic_devlink_unregister(struct ionic *ionic)
>   {
>          struct devlink *dl = priv_to_devlink(ionic);
> 
> -       devlink_unregister(dl);
>          devlink_port_unregister(&ionic->dl_port);
> +       devlink_unregister(dl);
>   }

I don't know about the rest of the drivers, but this seems to be the 
exact opposite of what Leon did in this patch over a year ago:
https://lore.kernel.org/netdev/cover.1632565508.git.leonro@nvidia.com/

I haven't kept up on all the discussion about this, but is there no 
longer a worry about registering the devlink object before all the 
related configuration bits are in place?

Does this open any potential issues with userland programs seeing the 
devlink device and trying to access port before they get registered?

sln