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

[Michal Kubecek]

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.kicin...@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