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

[Michal Kubecek <mkubecek@suse.cz>]

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@netronome.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()).

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.

Michal