Re: [PATCH] RDMA/siw: Remove direct link to net_device

[Leon Romanovsky <leon@kernel.org>]

On Tue, Dec 10, 2024 at 05:08:51PM +0000, Bernard Metzler wrote:
> 
> 
> > -----Original Message-----
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > Sent: Tuesday, December 10, 2024 3:56 PM
> > To: Bernard Metzler <BMT@zurich.ibm.com>
> > Cc: linux-rdma@vger.kernel.org; leon@kernel.org; linux-
> > kernel@vger.kernel.org; netdev@vger.kernel.org; syzkaller-
> > bugs@googlegroups.com; zyjzyj2000@gmail.com;
> > syzbot+4b87489410b4efd181bf@syzkaller.appspotmail.com
> > Subject: [EXTERNAL] Re: [PATCH] RDMA/siw: Remove direct link to net_device
> > 
> > On Tue, Dec 10, 2024 at 02:03:51PM +0100, Bernard Metzler wrote:
> > > diff --git a/drivers/infiniband/sw/siw/siw.h
> > b/drivers/infiniband/sw/siw/siw.h
> > > index 86d4d6a2170e..c8f75527b513 100644
> > > --- a/drivers/infiniband/sw/siw/siw.h
> > > +++ b/drivers/infiniband/sw/siw/siw.h
> > > @@ -69,16 +69,19 @@ struct siw_pd {
> > >
> > >  struct siw_device {
> > >  	struct ib_device base_dev;
> > > -	struct net_device *netdev;
> > >  	struct siw_dev_cap attrs;
> > >
> > >  	u32 vendor_part_id;
> > > +	struct {
> > > +		int ifindex;
> > 
> > ifindex is only stable so long as you are holding a reference on the
> > netdev..
> > > --- a/drivers/infiniband/sw/siw/siw_main.c
> > > +++ b/drivers/infiniband/sw/siw/siw_main.c
> > > @@ -287,7 +287,6 @@ static struct siw_device *siw_device_create(struct
> > net_device *netdev)
> > >  		return NULL;
> > >
> > >  	base_dev = &sdev->base_dev;
> > > -	sdev->netdev = netdev;
> > 
> > Like here needed to grab a reference before storing the pointer in the
> > sdev struct.
> > 
> 
> This patch was supposed to remove siw's link to netdev. So no
> reference to netdev would be needed. I did it under the
> assumption siw can locally keep all needed information up to
> date via netdev_event().
> But it seems the netdev itself can change during the lifetime of
> a siw device? With that ifindex would become wrong.
> 
> If the netdev can change without the siw driver being informed,
> holding a netdev pointer or reference seems useless.
>  
> So it would be best to always use ib_device_get_netdev() to get
> a valid netdev pointer, if current netdev info is needed by the
> driver?

Or call to dev_hold(netdev) in siw_device_create(). It will make sure
that netdev is stable.

Thanks

BTW, Need to check, maybe IB/core layer already called to dev_hold.

> 
> I just wanted to avoid the costly ib_device_get_netdev() call
> during query_qp(), query_port() and listen().
> 
> Thank you!
> Bernard.
>