netdev - Re: [PATCH net-next 4/5] nfp: add .ndo_get

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: <20190222090204.20c410e3@cakuba.netronome.com>
Date:   Fri, 22 Feb 2019 09:02:04 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     davem@...emloft.net, jiri@...nulli.us, andrew@...n.ch,
        f.fainelli@...il.com, netdev@...r.kernel.org,
        oss-drivers@...ronome.com
Subject: Re: [PATCH net-next 4/5] nfp: add .ndo_get_devlink

On Fri, 22 Feb 2019 11:04:50 +0100, Michal Kubecek wrote:
> On Thu, Feb 21, 2019 at 09:46:19AM -0800, Jakub Kicinski wrote:
> > Support getting devlink instance from a new NDO.
> > 
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
> > ---
> >  drivers/net/ethernet/netronome/nfp/nfp_app.h        |  2 ++
> >  drivers/net/ethernet/netronome/nfp/nfp_devlink.c    | 11 +++++++++++
> >  drivers/net/ethernet/netronome/nfp/nfp_net_common.c |  1 +
> >  drivers/net/ethernet/netronome/nfp/nfp_net_repr.c   |  1 +
> >  4 files changed, 15 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_app.h b/drivers/net/ethernet/netronome/nfp/nfp_app.h
> > index d578d856a009..f8d422713705 100644
> > --- a/drivers/net/ethernet/netronome/nfp/nfp_app.h
> > +++ b/drivers/net/ethernet/netronome/nfp/nfp_app.h
> > @@ -433,4 +433,6 @@ int nfp_app_nic_vnic_alloc(struct nfp_app *app, struct nfp_net *nn,
> >  int nfp_app_nic_vnic_init_phy_port(struct nfp_pf *pf, struct nfp_app *app,
> >  				   struct nfp_net *nn, unsigned int id);
> >  
> > +struct devlink *nfp_devlink_get_devlink(struct net_device *netdev);
> > +
> >  #endif
> > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> > index db2da99f6aa7..e9eca99cf493 100644
> > --- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> > +++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
> > @@ -376,3 +376,14 @@ void nfp_devlink_port_unregister(struct nfp_port *port)
> >  {
> >  	devlink_port_unregister(&port->dl_port);
> >  }
> > +
> > +struct devlink *nfp_devlink_get_devlink(struct net_device *netdev)
> > +{
> > +	struct nfp_app *app;
> > +
> > +	app = nfp_app_from_netdev(netdev);
> > +	if (!app)
> > +		return NULL;
> > +
> > +	return priv_to_devlink(app->pf);
> > +}  
> 
> AFAICS this would return a pointer to zero initialized struct devlink
> when built with CONFIG_DEVLINK=n. Then devlink_compat_running_version()
> would execute
> 
> 	if (!dev->netdev_ops->ndo_get_devlink)
> 		return;
> 
> 	devlink = dev->netdev_ops->ndo_get_devlink(dev);
> 	if (!devlink || !devlink->ops->info_get)
> 		return;
> 
> with non-null devlink but null devlink->ops so that it dereferences null
> pointer (and so does devlink_compat_flash_update()).

devlink_compat_flash_update() is this if CONFIG_DEVLINK=n:

static inline int
devlink_compat_flash_update(struct net_device *dev, const char *file_name)
{
	return -EOPNOTSUPP;
}

No?

> Maybe it would be safer not to call ndo_get_devlink directly and have
> an inline wrapper like
> 
> #if IS_ENABLED(CONFIG_NET_DEVLINK)
> static inline struct devlink *dev_get_devlink(struct net_device *dev)
> {
> 	if (dev->netdev_ops->ndo_get_devlink)
> 		return dev->netdev_ops->ndo_get_devlink();
> 	else
> 		retrurn NULL;
> }
> #else
> static inline struct devlink *dev_get_devlink(struct net_device *dev)
> {
> 	return NULL;
> }
> #endif
> 
> so that one can simply call the wrapper and check return value for NULL.

Only devlink code can call this ndo, and it doesn't exist with
DEVLINK=n.  I don't dislike wrappers for NDOs, but I'll defer to Jiri
to decide if we want a wrapper here (without the #if/#else, just the
first part for code clarity) :)