Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Halil Pasic]

On Wed, 6 Nov 2024 15:59:10 +0200
Leon Romanovsky <leon@kernel.org> wrote:

> > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?  
> 
> RDMA core code is drivers/infiniband/core/*.

Understood. So this is a violation of the no direct access to the
callbacks rule.

> 
> > I would guess it is not, and I would not actually mind sending a patch
> > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > ksmbd_rdma_capable_netdev()").  
> 
> It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> GID, netdev and fabric complexity.

I'm not familiar enough with either of the subsystems. Based on your
answer my guess is that it ain't outright bugous but still a layering 
violation. Copying linux-cifs@vger.kernel.org so that 
the smb are aware.

Thank you very much for all the explanations!

Regards,
Halil 

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Thu, Nov 07, 2024 at 12:56:43PM +0100, Halil Pasic wrote:
> On Wed, 6 Nov 2024 15:59:10 +0200
> Leon Romanovsky <leon@kernel.org> wrote:
> 
> > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?  
> > 
> > RDMA core code is drivers/infiniband/core/*.
> 
> Understood. So this is a violation of the no direct access to the
> callbacks rule.

It is not rule, but more common sense. Callbacks don't provide any
module reference counting, module autoload e.t.c

It is very rare situation where you call device callbacks from one subsystem
in another. I'm not familiar with such situations.

> 
> > 
> > > I would guess it is not, and I would not actually mind sending a patch
> > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > > ksmbd_rdma_capable_netdev()").  
> > 
> > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > GID, netdev and fabric complexity.
> 
> I'm not familiar enough with either of the subsystems. Based on your
> answer my guess is that it ain't outright bugous but still a layering 
> violation. Copying linux-cifs@vger.kernel.org so that 
> the smb are aware.
> 
> Thank you very much for all the explanations!
> 
> Regards,
> Halil 

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Namjae Jeon]

On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
>
> On Wed, 6 Nov 2024 15:59:10 +0200
> Leon Romanovsky <leon@kernel.org> wrote:
>
> > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> >
> > RDMA core code is drivers/infiniband/core/*.
>
> Understood. So this is a violation of the no direct access to the
> callbacks rule.
>
> >
> > > I would guess it is not, and I would not actually mind sending a patch
> > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > > ksmbd_rdma_capable_netdev()").
> >
> > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > GID, netdev and fabric complexity.
>
> I'm not familiar enough with either of the subsystems. Based on your
> answer my guess is that it ain't outright bugous but still a layering
> violation. Copying linux-cifs@vger.kernel.org so that
> the smb are aware.
Could you please elaborate what the violation is ?
I would also appreciate it if you could suggest to me how to fix this.

Thanks.
>
> Thank you very much for all the explanations!
>
> Regards,
> Halil
>

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> >
> > On Wed, 6 Nov 2024 15:59:10 +0200
> > Leon Romanovsky <leon@kernel.org> wrote:
> >
> > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> > >
> > > RDMA core code is drivers/infiniband/core/*.
> >
> > Understood. So this is a violation of the no direct access to the
> > callbacks rule.
> >
> > >
> > > > I would guess it is not, and I would not actually mind sending a patch
> > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > > > ksmbd_rdma_capable_netdev()").
> > >
> > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > > GID, netdev and fabric complexity.
> >
> > I'm not familiar enough with either of the subsystems. Based on your
> > answer my guess is that it ain't outright bugous but still a layering
> > violation. Copying linux-cifs@vger.kernel.org so that
> > the smb are aware.
> Could you please elaborate what the violation is ?

There are many, but the most screaming is that ksmbd has logic to
differentiate IPoIB devices. These devices are pure netdev devices
and should be treated like that. ULPs should treat them exactly
as they treat netdev devices.

> I would also appreciate it if you could suggest to me how to fix this.
> 
> Thanks.
> >
> > Thank you very much for all the explanations!
> >
> > Regards,
> > Halil
> >

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Namjae Jeon]

On Sat, Nov 9, 2024 at 2:59 AM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> > >
> > > On Wed, 6 Nov 2024 15:59:10 +0200
> > > Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> > > >
> > > > RDMA core code is drivers/infiniband/core/*.
> > >
> > > Understood. So this is a violation of the no direct access to the
> > > callbacks rule.
> > >
> > > >
> > > > > I would guess it is not, and I would not actually mind sending a patch
> > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > > > > ksmbd_rdma_capable_netdev()").
> > > >
> > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > > > GID, netdev and fabric complexity.
> > >
> > > I'm not familiar enough with either of the subsystems. Based on your
> > > answer my guess is that it ain't outright bugous but still a layering
> > > violation. Copying linux-cifs@vger.kernel.org so that
> > > the smb are aware.
> > Could you please elaborate what the violation is ?
>
> There are many, but the most screaming is that ksmbd has logic to
> differentiate IPoIB devices. These devices are pure netdev devices
> and should be treated like that. ULPs should treat them exactly
> as they treat netdev devices.
Okay, I'll discuss with Kangjing if there's another way to avoid this issue.
If not, I'll revert the patch.

Thanks.
>
> > I would also appreciate it if you could suggest to me how to fix this.
> >
> > Thanks.
> > >
> > > Thank you very much for all the explanations!
> > >
> > > Regards,
> > > Halil
> > >

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Kangjing Huang]

Hi there,

I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
as mentioned in the thread.

I am working on modifying the patch to take care of the layering
violation. The original patch was meant to fix an issue with ksmbd,
where an IPoIB netdev was not recognized as RDMA-capable. The original
version of the capability evaluation tries to match each netdev to
ib_device by calling get_netdev in ib verbs. However this only works
in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
and since with IPoIB it is the other way around (netdev is the upper
layer of ib_device), get_netdev won't work anymore.

I tried to replicate the behavior of device matching reversely in the
original version of my patch using GID, which ended up as the layering
violation. However I am unaware of any exported functions from the
IPoIB driver that could do the reverse lookup from netdev to the lower
layer ib_device. Actually it seems that the IPoIB driver does not have
any exported symbols at all.

It might be that the device matching in reverse just does not make any
sense and does not need to be done at all. As long as it is an IPoIB
device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
automatically assume it is RDMA-capable. I am not 100% sure about this
though.

I am uncertain about how to proceed at this point and would like to
know your thoughts and opinions on this.

Thanks,
Kangjing

On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> > >
> > > On Wed, 6 Nov 2024 15:59:10 +0200
> > > Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> > > >
> > > > RDMA core code is drivers/infiniband/core/*.
> > >
> > > Understood. So this is a violation of the no direct access to the
> > > callbacks rule.
> > >
> > > >
> > > > > I would guess it is not, and I would not actually mind sending a patch
> > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > > > > ksmbd_rdma_capable_netdev()").
> > > >
> > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > > > GID, netdev and fabric complexity.
> > >
> > > I'm not familiar enough with either of the subsystems. Based on your
> > > answer my guess is that it ain't outright bugous but still a layering
> > > violation. Copying linux-cifs@vger.kernel.org so that
> > > the smb are aware.
> > Could you please elaborate what the violation is ?
>
> There are many, but the most screaming is that ksmbd has logic to
> differentiate IPoIB devices. These devices are pure netdev devices
> and should be treated like that. ULPs should treat them exactly
> as they treat netdev devices.
>
> > I would also appreciate it if you could suggest to me how to fix this.
> >
> > Thanks.
> > >
> > > Thank you very much for all the explanations!
> > >
> > > Regards,
> > > Halil
> > >



-- 
Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Fri, Dec 13, 2024, at 13:07, Kangjing Huang wrote:
> Hi there,
>
> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
> as mentioned in the thread.
>
> I am working on modifying the patch to take care of the layering
> violation. The original patch was meant to fix an issue with ksmbd,
> where an IPoIB netdev was not recognized as RDMA-capable.

This is exactly the purpose and design of IPoIB, to present regular netdev to the users and hide IB layer from them.

> The original
> version of the capability evaluation tries to match each netdev to
> ib_device by calling get_netdev in ib verbs. However this only works
> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
> and since with IPoIB it is the other way around (netdev is the upper
> layer of ib_device), get_netdev won't work anymore.
>
> I tried to replicate the behavior of device matching reversely in the
> original version of my patch using GID, which ended up as the layering
> violation. However I am unaware of any exported functions from the
> IPoIB driver that could do the reverse lookup from netdev to the lower
> layer ib_device. Actually it seems that the IPoIB driver does not have
> any exported symbols at all.
>
> It might be that the device matching in reverse just does not make any
> sense and does not need to be done at all. As long as it is an IPoIB
> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
> automatically assume it is RDMA-capable. I am not 100% sure about this
> though.
>
> I am uncertain about how to proceed at this point and would like to
> know your thoughts and opinions on this.

Delete this code completely and make sure that ksmbd has two paths only. One for netdevs and one for ib_devices. It is upto users to decide on which interface to run.

Thanks 

>
> Thanks,
> Kangjing
>
> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
>>
>> On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
>> > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
>> > >
>> > > On Wed, 6 Nov 2024 15:59:10 +0200
>> > > Leon Romanovsky <leon@kernel.org> wrote:
>> > >
>> > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
>> > > >
>> > > > RDMA core code is drivers/infiniband/core/*.
>> > >
>> > > Understood. So this is a violation of the no direct access to the
>> > > callbacks rule.
>> > >
>> > > >
>> > > > > I would guess it is not, and I would not actually mind sending a patch
>> > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
>> > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
>> > > > > ksmbd_rdma_capable_netdev()").
>> > > >
>> > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
>> > > > GID, netdev and fabric complexity.
>> > >
>> > > I'm not familiar enough with either of the subsystems. Based on your
>> > > answer my guess is that it ain't outright bugous but still a layering
>> > > violation. Copying linux-cifs@vger.kernel.org so that
>> > > the smb are aware.
>> > Could you please elaborate what the violation is ?
>>
>> There are many, but the most screaming is that ksmbd has logic to
>> differentiate IPoIB devices. These devices are pure netdev devices
>> and should be treated like that. ULPs should treat them exactly
>> as they treat netdev devices.
>>
>> > I would also appreciate it if you could suggest to me how to fix this.
>> >
>> > Thanks.
>> > >
>> > > Thank you very much for all the explanations!
>> > >
>> > > Regards,
>> > > Halil
>> > >
>
>
>
> -- 
> Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Namjae Jeon]

On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
>
> Hi there,
>
> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
> as mentioned in the thread.
>
> I am working on modifying the patch to take care of the layering
> violation. The original patch was meant to fix an issue with ksmbd,
> where an IPoIB netdev was not recognized as RDMA-capable. The original
> version of the capability evaluation tries to match each netdev to
> ib_device by calling get_netdev in ib verbs. However this only works
> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
> and since with IPoIB it is the other way around (netdev is the upper
> layer of ib_device), get_netdev won't work anymore.
>
> I tried to replicate the behavior of device matching reversely in the
> original version of my patch using GID, which ended up as the layering
> violation. However I am unaware of any exported functions from the
> IPoIB driver that could do the reverse lookup from netdev to the lower
> layer ib_device. Actually it seems that the IPoIB driver does not have
> any exported symbols at all.
>
> It might be that the device matching in reverse just does not make any
> sense and does not need to be done at all. As long as it is an IPoIB
> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
> automatically assume it is RDMA-capable. I am not 100% sure about this
> though.
Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
How about assuming it's RDMA-capable and allowing users to turn
RDMA-capable on/off via sysfs?

Thanks!
>
> I am uncertain about how to proceed at this point and would like to
> know your thoughts and opinions on this.
>
> Thanks,
> Kangjing
>
> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> > > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> > > >
> > > > On Wed, 6 Nov 2024 15:59:10 +0200
> > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> > > > >
> > > > > RDMA core code is drivers/infiniband/core/*.
> > > >
> > > > Understood. So this is a violation of the no direct access to the
> > > > callbacks rule.
> > > >
> > > > >
> > > > > > I would guess it is not, and I would not actually mind sending a patch
> > > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > > > > > ksmbd_rdma_capable_netdev()").
> > > > >
> > > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > > > > GID, netdev and fabric complexity.
> > > >
> > > > I'm not familiar enough with either of the subsystems. Based on your
> > > > answer my guess is that it ain't outright bugous but still a layering
> > > > violation. Copying linux-cifs@vger.kernel.org so that
> > > > the smb are aware.
> > > Could you please elaborate what the violation is ?
> >
> > There are many, but the most screaming is that ksmbd has logic to
> > differentiate IPoIB devices. These devices are pure netdev devices
> > and should be treated like that. ULPs should treat them exactly
> > as they treat netdev devices.
> >
> > > I would also appreciate it if you could suggest to me how to fix this.
> > >
> > > Thanks.
> > > >
> > > > Thank you very much for all the explanations!
> > > >
> > > > Regards,
> > > > Halil
> > > >
>
>
>
> --
> Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Sat, Dec 14, 2024, at 04:33, Namjae Jeon wrote:
> On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
>>
>> Hi there,
>>
>> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
>> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
>> as mentioned in the thread.
>>
>> I am working on modifying the patch to take care of the layering
>> violation. The original patch was meant to fix an issue with ksmbd,
>> where an IPoIB netdev was not recognized as RDMA-capable. The original
>> version of the capability evaluation tries to match each netdev to
>> ib_device by calling get_netdev in ib verbs. However this only works
>> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
>> and since with IPoIB it is the other way around (netdev is the upper
>> layer of ib_device), get_netdev won't work anymore.
>>
>> I tried to replicate the behavior of device matching reversely in the
>> original version of my patch using GID, which ended up as the layering
>> violation. However I am unaware of any exported functions from the
>> IPoIB driver that could do the reverse lookup from netdev to the lower
>> layer ib_device. Actually it seems that the IPoIB driver does not have
>> any exported symbols at all.
>>
>> It might be that the device matching in reverse just does not make any
>> sense and does not need to be done at all. As long as it is an IPoIB
>> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
>> automatically assume it is RDMA-capable. I am not 100% sure about this
>> though.
> Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
> How about assuming it's RDMA-capable and allowing users to turn
> RDMA-capable on/off via sysfs?

Any attempt to treat ipoib differently from regular netdevice is wrong by definition.

>
> Thanks!
>>
>> I am uncertain about how to proceed at this point and would like to
>> know your thoughts and opinions on this.
>>
>> Thanks,
>> Kangjing
>>
>> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
>> >
>> > On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
>> > > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
>> > > >
>> > > > On Wed, 6 Nov 2024 15:59:10 +0200
>> > > > Leon Romanovsky <leon@kernel.org> wrote:
>> > > >
>> > > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
>> > > > >
>> > > > > RDMA core code is drivers/infiniband/core/*.
>> > > >
>> > > > Understood. So this is a violation of the no direct access to the
>> > > > callbacks rule.
>> > > >
>> > > > >
>> > > > > > I would guess it is not, and I would not actually mind sending a patch
>> > > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
>> > > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
>> > > > > > ksmbd_rdma_capable_netdev()").
>> > > > >
>> > > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
>> > > > > GID, netdev and fabric complexity.
>> > > >
>> > > > I'm not familiar enough with either of the subsystems. Based on your
>> > > > answer my guess is that it ain't outright bugous but still a layering
>> > > > violation. Copying linux-cifs@vger.kernel.org so that
>> > > > the smb are aware.
>> > > Could you please elaborate what the violation is ?
>> >
>> > There are many, but the most screaming is that ksmbd has logic to
>> > differentiate IPoIB devices. These devices are pure netdev devices
>> > and should be treated like that. ULPs should treat them exactly
>> > as they treat netdev devices.
>> >
>> > > I would also appreciate it if you could suggest to me how to fix this.
>> > >
>> > > Thanks.
>> > > >
>> > > > Thank you very much for all the explanations!
>> > > >
>> > > > Regards,
>> > > > Halil
>> > > >
>>
>>
>>
>> --
>> Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Kangjing Huang]

On Sat, Dec 14, 2024 at 1:06 AM Leon Romanovsky <leon@kernel.org> wrote:
>
>
>
> On Sat, Dec 14, 2024, at 04:33, Namjae Jeon wrote:
> > On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
> >>
> >> Hi there,
> >>
> >> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
> >> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
> >> as mentioned in the thread.
> >>
> >> I am working on modifying the patch to take care of the layering
> >> violation. The original patch was meant to fix an issue with ksmbd,
> >> where an IPoIB netdev was not recognized as RDMA-capable. The original
> >> version of the capability evaluation tries to match each netdev to
> >> ib_device by calling get_netdev in ib verbs. However this only works
> >> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
> >> and since with IPoIB it is the other way around (netdev is the upper
> >> layer of ib_device), get_netdev won't work anymore.
> >>
> >> I tried to replicate the behavior of device matching reversely in the
> >> original version of my patch using GID, which ended up as the layering
> >> violation. However I am unaware of any exported functions from the
> >> IPoIB driver that could do the reverse lookup from netdev to the lower
> >> layer ib_device. Actually it seems that the IPoIB driver does not have
> >> any exported symbols at all.
> >>
> >> It might be that the device matching in reverse just does not make any
> >> sense and does not need to be done at all. As long as it is an IPoIB
> >> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
> >> automatically assume it is RDMA-capable. I am not 100% sure about this
> >> though.
> > Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
> > How about assuming it's RDMA-capable and allowing users to turn
> > RDMA-capable on/off via sysfs?
It does make more sense to me at this point to just broadly assume all
ARPHRD_INFINIBAND types to be RDMA-capable, we just need to make sure
this assumption indeed holds and figure out to what extent this could
involve the same layering violation.

>
> Any attempt to treat ipoib differently from regular netdevice is wrong by definition.
>
I would agree that the design direction to treat ipoib as a pure
regular net_device is the good way to go. But the problem with ksmbd
and ipoib devices stems from the SMB protocol itself.

In contrast to protocols that focus on certain functionalities like
nfs, SMB actually tries to manage network interfaces actively in the
protocol itself: SMB protocol's RDMA support (dubbed SMB Direct) is a
sub-feature of SMB Multichannel. Multichannel is designed to let
client and server find multiple data paths automatically (imagine a
pair of hosts with multiple adapters connected by multiple cables) to
increase bandwidth. So client can initiate a
FSCTL_QUERY_NETWORK_INTERFACE_INFO request and server is expected to
respond with NETWORK_INTERFACE_INFO containing _all_ local network
interface informations, including their capabilities such as
RDMA_CAPABLE (for details see ref [MS-SMB2] 3.3.5.15.11) Only upon
seeing the capability flag would a client attempt to initiate a RDMA
connection.

Reference: [MS-SMB2](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d.pdf)

TLDR is that the SMB protocol requires the server to enumerate all
net_devices and indicate their RDMA capability, and
ksmbd_rdma_capable_netdev() is only used in that process. Given such
context, I wonder what should be the best way to approach this? Is
using ARPHRD_INFINIBAND good enough and acceptable in terms of
layering?

> >
> > Thanks!
> >>
> >> I am uncertain about how to proceed at this point and would like to
> >> know your thoughts and opinions on this.
> >>
> >> Thanks,
> >> Kangjing
> >>
> >> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
> >> >
> >> > On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> >> > > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> >> > > >
> >> > > > On Wed, 6 Nov 2024 15:59:10 +0200
> >> > > > Leon Romanovsky <leon@kernel.org> wrote:
> >> > > >
> >> > > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> >> > > > >
> >> > > > > RDMA core code is drivers/infiniband/core/*.
> >> > > >
> >> > > > Understood. So this is a violation of the no direct access to the
> >> > > > callbacks rule.
> >> > > >
> >> > > > >
> >> > > > > > I would guess it is not, and I would not actually mind sending a patch
> >> > > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> >> > > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> >> > > > > > ksmbd_rdma_capable_netdev()").
> >> > > > >
> >> > > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> >> > > > > GID, netdev and fabric complexity.
> >> > > >
> >> > > > I'm not familiar enough with either of the subsystems. Based on your
> >> > > > answer my guess is that it ain't outright bugous but still a layering
> >> > > > violation. Copying linux-cifs@vger.kernel.org so that
> >> > > > the smb are aware.
> >> > > Could you please elaborate what the violation is ?
> >> >
> >> > There are many, but the most screaming is that ksmbd has logic to
> >> > differentiate IPoIB devices. These devices are pure netdev devices
> >> > and should be treated like that. ULPs should treat them exactly
> >> > as they treat netdev devices.
> >> >
> >> > > I would also appreciate it if you could suggest to me how to fix this.
> >> > >
> >> > > Thanks.
> >> > > >
> >> > > > Thank you very much for all the explanations!
> >> > > >
> >> > > > Regards,
> >> > > > Halil
> >> > > >
> >>
> >>
> >>
> >> --
> >> Kangjing "Chaser" Huang



--
Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Sat, Dec 14, 2024 at 08:02:14AM +0000, Kangjing Huang wrote:
> On Sat, Dec 14, 2024 at 1:06 AM Leon Romanovsky <leon@kernel.org> wrote:
> >
> >
> >
> > On Sat, Dec 14, 2024, at 04:33, Namjae Jeon wrote:
> > > On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
> > >>
> > >> Hi there,
> > >>
> > >> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
> > >> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
> > >> as mentioned in the thread.
> > >>
> > >> I am working on modifying the patch to take care of the layering
> > >> violation. The original patch was meant to fix an issue with ksmbd,
> > >> where an IPoIB netdev was not recognized as RDMA-capable. The original
> > >> version of the capability evaluation tries to match each netdev to
> > >> ib_device by calling get_netdev in ib verbs. However this only works
> > >> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
> > >> and since with IPoIB it is the other way around (netdev is the upper
> > >> layer of ib_device), get_netdev won't work anymore.
> > >>
> > >> I tried to replicate the behavior of device matching reversely in the
> > >> original version of my patch using GID, which ended up as the layering
> > >> violation. However I am unaware of any exported functions from the
> > >> IPoIB driver that could do the reverse lookup from netdev to the lower
> > >> layer ib_device. Actually it seems that the IPoIB driver does not have
> > >> any exported symbols at all.
> > >>
> > >> It might be that the device matching in reverse just does not make any
> > >> sense and does not need to be done at all. As long as it is an IPoIB
> > >> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
> > >> automatically assume it is RDMA-capable. I am not 100% sure about this
> > >> though.
> > > Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
> > > How about assuming it's RDMA-capable and allowing users to turn
> > > RDMA-capable on/off via sysfs?
> It does make more sense to me at this point to just broadly assume all
> ARPHRD_INFINIBAND types to be RDMA-capable, we just need to make sure
> this assumption indeed holds and figure out to what extent this could
> involve the same layering violation.
> 
> >
> > Any attempt to treat ipoib differently from regular netdevice is wrong by definition.
> >
> I would agree that the design direction to treat ipoib as a pure
> regular net_device is the good way to go. But the problem with ksmbd
> and ipoib devices stems from the SMB protocol itself.
> 
> In contrast to protocols that focus on certain functionalities like
> nfs, SMB actually tries to manage network interfaces actively in the
> protocol itself: SMB protocol's RDMA support (dubbed SMB Direct) is a
> sub-feature of SMB Multichannel. Multichannel is designed to let
> client and server find multiple data paths automatically (imagine a
> pair of hosts with multiple adapters connected by multiple cables) to
> increase bandwidth. So client can initiate a
> FSCTL_QUERY_NETWORK_INTERFACE_INFO request and server is expected to
> respond with NETWORK_INTERFACE_INFO containing _all_ local network
> interface informations, including their capabilities such as
> RDMA_CAPABLE (for details see ref [MS-SMB2] 3.3.5.15.11) Only upon
> seeing the capability flag would a client attempt to initiate a RDMA
> connection.
> 
> Reference: [MS-SMB2](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d.pdf)
> 
> TLDR is that the SMB protocol requires the server to enumerate all
> net_devices and indicate their RDMA capability, and
> ksmbd_rdma_capable_netdev() is only used in that process. Given such
> context, I wonder what should be the best way to approach this? Is
> using ARPHRD_INFINIBAND good enough and acceptable in terms of
> layering?

The thing is that ARPHRD_INFINIBAND indeed represent IPoIB and it is
right check if netdev is IPoIB or not. The layering problem is that
upper layers (ULPs) should use it as regular netdevice.

Thanks

> 
> > >
> > > Thanks!
> > >>
> > >> I am uncertain about how to proceed at this point and would like to
> > >> know your thoughts and opinions on this.
> > >>
> > >> Thanks,
> > >> Kangjing
> > >>
> > >> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >> >
> > >> > On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> > >> > > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> > >> > > >
> > >> > > > On Wed, 6 Nov 2024 15:59:10 +0200
> > >> > > > Leon Romanovsky <leon@kernel.org> wrote:
> > >> > > >
> > >> > > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> > >> > > > >
> > >> > > > > RDMA core code is drivers/infiniband/core/*.
> > >> > > >
> > >> > > > Understood. So this is a violation of the no direct access to the
> > >> > > > callbacks rule.
> > >> > > >
> > >> > > > >
> > >> > > > > > I would guess it is not, and I would not actually mind sending a patch
> > >> > > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > >> > > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > >> > > > > > ksmbd_rdma_capable_netdev()").
> > >> > > > >
> > >> > > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > >> > > > > GID, netdev and fabric complexity.
> > >> > > >
> > >> > > > I'm not familiar enough with either of the subsystems. Based on your
> > >> > > > answer my guess is that it ain't outright bugous but still a layering
> > >> > > > violation. Copying linux-cifs@vger.kernel.org so that
> > >> > > > the smb are aware.
> > >> > > Could you please elaborate what the violation is ?
> > >> >
> > >> > There are many, but the most screaming is that ksmbd has logic to
> > >> > differentiate IPoIB devices. These devices are pure netdev devices
> > >> > and should be treated like that. ULPs should treat them exactly
> > >> > as they treat netdev devices.
> > >> >
> > >> > > I would also appreciate it if you could suggest to me how to fix this.
> > >> > >
> > >> > > Thanks.
> > >> > > >
> > >> > > > Thank you very much for all the explanations!
> > >> > > >
> > >> > > > Regards,
> > >> > > > Halil
> > >> > > >
> > >>
> > >>
> > >>
> > >> --
> > >> Kangjing "Chaser" Huang
> 
> 
> 
> --
> Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Kangjing Huang]

On Thu, Dec 19, 2024 at 4:56 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Sat, Dec 14, 2024 at 08:02:14AM +0000, Kangjing Huang wrote:
> > On Sat, Dec 14, 2024 at 1:06 AM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > >
> > >
> > > On Sat, Dec 14, 2024, at 04:33, Namjae Jeon wrote:
> > > > On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
> > > >>
> > > >> Hi there,
> > > >>
> > > >> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
> > > >> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
> > > >> as mentioned in the thread.
> > > >>
> > > >> I am working on modifying the patch to take care of the layering
> > > >> violation. The original patch was meant to fix an issue with ksmbd,
> > > >> where an IPoIB netdev was not recognized as RDMA-capable. The original
> > > >> version of the capability evaluation tries to match each netdev to
> > > >> ib_device by calling get_netdev in ib verbs. However this only works
> > > >> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
> > > >> and since with IPoIB it is the other way around (netdev is the upper
> > > >> layer of ib_device), get_netdev won't work anymore.
> > > >>
> > > >> I tried to replicate the behavior of device matching reversely in the
> > > >> original version of my patch using GID, which ended up as the layering
> > > >> violation. However I am unaware of any exported functions from the
> > > >> IPoIB driver that could do the reverse lookup from netdev to the lower
> > > >> layer ib_device. Actually it seems that the IPoIB driver does not have
> > > >> any exported symbols at all.
> > > >>
> > > >> It might be that the device matching in reverse just does not make any
> > > >> sense and does not need to be done at all. As long as it is an IPoIB
> > > >> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
> > > >> automatically assume it is RDMA-capable. I am not 100% sure about this
> > > >> though.
> > > > Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
> > > > How about assuming it's RDMA-capable and allowing users to turn
> > > > RDMA-capable on/off via sysfs?
> > It does make more sense to me at this point to just broadly assume all
> > ARPHRD_INFINIBAND types to be RDMA-capable, we just need to make sure
> > this assumption indeed holds and figure out to what extent this could
> > involve the same layering violation.
> >
> > >
> > > Any attempt to treat ipoib differently from regular netdevice is wrong by definition.
> > >
> > I would agree that the design direction to treat ipoib as a pure
> > regular net_device is the good way to go. But the problem with ksmbd
> > and ipoib devices stems from the SMB protocol itself.
> >
> > In contrast to protocols that focus on certain functionalities like
> > nfs, SMB actually tries to manage network interfaces actively in the
> > protocol itself: SMB protocol's RDMA support (dubbed SMB Direct) is a
> > sub-feature of SMB Multichannel. Multichannel is designed to let
> > client and server find multiple data paths automatically (imagine a
> > pair of hosts with multiple adapters connected by multiple cables) to
> > increase bandwidth. So client can initiate a
> > FSCTL_QUERY_NETWORK_INTERFACE_INFO request and server is expected to
> > respond with NETWORK_INTERFACE_INFO containing _all_ local network
> > interface informations, including their capabilities such as
> > RDMA_CAPABLE (for details see ref [MS-SMB2] 3.3.5.15.11) Only upon
> > seeing the capability flag would a client attempt to initiate a RDMA
> > connection.
> >
> > Reference: [MS-SMB2](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d.pdf)
> >
> > TLDR is that the SMB protocol requires the server to enumerate all
> > net_devices and indicate their RDMA capability, and
> > ksmbd_rdma_capable_netdev() is only used in that process. Given such
> > context, I wonder what should be the best way to approach this? Is
> > using ARPHRD_INFINIBAND good enough and acceptable in terms of
> > layering?
>

> The thing is that ARPHRD_INFINIBAND indeed represent IPoIB and it is
> right check if netdev is IPoIB or not. The layering problem is that
> upper layers (ULPs) should use it as regular netdevice.

This is good to know. However, since the SMB protocol explicitly calls
for enumeration of all network interfaces on the server host,
including their RDMA capabilities, I believe this is a sensible
exception to the layering rule. Or is there anyway else to do this
enumeration from the kernel space?

Or we can give up implementing the full spec of the SMB protocol and
call for explicit configuration from user space on how to respond to
the IOCTL requests in question. Which one looks more sensible to you?

Thanks

>
> Thanks
>
> >
> > > >
> > > > Thanks!
> > > >>
> > > >> I am uncertain about how to proceed at this point and would like to
> > > >> know your thoughts and opinions on this.
> > > >>
> > > >> Thanks,
> > > >> Kangjing
> > > >>
> > > >> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > >> >
> > > >> > On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> > > >> > > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> > > >> > > >
> > > >> > > > On Wed, 6 Nov 2024 15:59:10 +0200
> > > >> > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > >> > > >
> > > >> > > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> > > >> > > > >
> > > >> > > > > RDMA core code is drivers/infiniband/core/*.
> > > >> > > >
> > > >> > > > Understood. So this is a violation of the no direct access to the
> > > >> > > > callbacks rule.
> > > >> > > >
> > > >> > > > >
> > > >> > > > > > I would guess it is not, and I would not actually mind sending a patch
> > > >> > > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > > >> > > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > > >> > > > > > ksmbd_rdma_capable_netdev()").
> > > >> > > > >
> > > >> > > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > > >> > > > > GID, netdev and fabric complexity.
> > > >> > > >
> > > >> > > > I'm not familiar enough with either of the subsystems. Based on your
> > > >> > > > answer my guess is that it ain't outright bugous but still a layering
> > > >> > > > violation. Copying linux-cifs@vger.kernel.org so that
> > > >> > > > the smb are aware.
> > > >> > > Could you please elaborate what the violation is ?
> > > >> >
> > > >> > There are many, but the most screaming is that ksmbd has logic to
> > > >> > differentiate IPoIB devices. These devices are pure netdev devices
> > > >> > and should be treated like that. ULPs should treat them exactly
> > > >> > as they treat netdev devices.
> > > >> >
> > > >> > > I would also appreciate it if you could suggest to me how to fix this.
> > > >> > >
> > > >> > > Thanks.
> > > >> > > >
> > > >> > > > Thank you very much for all the explanations!
> > > >> > > >
> > > >> > > > Regards,
> > > >> > > > Halil
> > > >> > > >
> > > >>
> > > >>
> > > >>
> > > >> --
> > > >> Kangjing "Chaser" Huang
> >
> >
> >
> > --
> > Kangjing "Chaser" Huang



-- 
Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Tue, Jan 07, 2025 at 10:51:19PM +0000, Kangjing Huang wrote:
> On Thu, Dec 19, 2024 at 4:56 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Sat, Dec 14, 2024 at 08:02:14AM +0000, Kangjing Huang wrote:
> > > On Sat, Dec 14, 2024 at 1:06 AM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > >
> > > >
> > > > On Sat, Dec 14, 2024, at 04:33, Namjae Jeon wrote:
> > > > > On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
> > > > >>
> > > > >> Hi there,
> > > > >>
> > > > >> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
> > > > >> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
> > > > >> as mentioned in the thread.
> > > > >>
> > > > >> I am working on modifying the patch to take care of the layering
> > > > >> violation. The original patch was meant to fix an issue with ksmbd,
> > > > >> where an IPoIB netdev was not recognized as RDMA-capable. The original
> > > > >> version of the capability evaluation tries to match each netdev to
> > > > >> ib_device by calling get_netdev in ib verbs. However this only works
> > > > >> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
> > > > >> and since with IPoIB it is the other way around (netdev is the upper
> > > > >> layer of ib_device), get_netdev won't work anymore.
> > > > >>
> > > > >> I tried to replicate the behavior of device matching reversely in the
> > > > >> original version of my patch using GID, which ended up as the layering
> > > > >> violation. However I am unaware of any exported functions from the
> > > > >> IPoIB driver that could do the reverse lookup from netdev to the lower
> > > > >> layer ib_device. Actually it seems that the IPoIB driver does not have
> > > > >> any exported symbols at all.
> > > > >>
> > > > >> It might be that the device matching in reverse just does not make any
> > > > >> sense and does not need to be done at all. As long as it is an IPoIB
> > > > >> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
> > > > >> automatically assume it is RDMA-capable. I am not 100% sure about this
> > > > >> though.
> > > > > Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
> > > > > How about assuming it's RDMA-capable and allowing users to turn
> > > > > RDMA-capable on/off via sysfs?
> > > It does make more sense to me at this point to just broadly assume all
> > > ARPHRD_INFINIBAND types to be RDMA-capable, we just need to make sure
> > > this assumption indeed holds and figure out to what extent this could
> > > involve the same layering violation.
> > >
> > > >
> > > > Any attempt to treat ipoib differently from regular netdevice is wrong by definition.
> > > >
> > > I would agree that the design direction to treat ipoib as a pure
> > > regular net_device is the good way to go. But the problem with ksmbd
> > > and ipoib devices stems from the SMB protocol itself.
> > >
> > > In contrast to protocols that focus on certain functionalities like
> > > nfs, SMB actually tries to manage network interfaces actively in the
> > > protocol itself: SMB protocol's RDMA support (dubbed SMB Direct) is a
> > > sub-feature of SMB Multichannel. Multichannel is designed to let
> > > client and server find multiple data paths automatically (imagine a
> > > pair of hosts with multiple adapters connected by multiple cables) to
> > > increase bandwidth. So client can initiate a
> > > FSCTL_QUERY_NETWORK_INTERFACE_INFO request and server is expected to
> > > respond with NETWORK_INTERFACE_INFO containing _all_ local network
> > > interface informations, including their capabilities such as
> > > RDMA_CAPABLE (for details see ref [MS-SMB2] 3.3.5.15.11) Only upon
> > > seeing the capability flag would a client attempt to initiate a RDMA
> > > connection.
> > >
> > > Reference: [MS-SMB2](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d.pdf)
> > >
> > > TLDR is that the SMB protocol requires the server to enumerate all
> > > net_devices and indicate their RDMA capability, and
> > > ksmbd_rdma_capable_netdev() is only used in that process. Given such
> > > context, I wonder what should be the best way to approach this? Is
> > > using ARPHRD_INFINIBAND good enough and acceptable in terms of
> > > layering?
> >
> 
> > The thing is that ARPHRD_INFINIBAND indeed represent IPoIB and it is
> > right check if netdev is IPoIB or not. The layering problem is that
> > upper layers (ULPs) should use it as regular netdevice.
> 
> This is good to know. However, since the SMB protocol explicitly calls
> for enumeration of all network interfaces on the server host,
> including their RDMA capabilities, I believe this is a sensible
> exception to the layering rule. Or is there anyway else to do this
> enumeration from the kernel space?
> 
> Or we can give up implementing the full spec of the SMB protocol and
> call for explicit configuration from user space on how to respond to
> the IOCTL requests in question. Which one looks more sensible to you?

My preference is to have same IPoIB treatment for all ULPs, including SMB.

My GUESS is that SMB specification authors didn't take into account HW and
Linux SW development around IPoIB and weren't aware of IPoIB offload which
is implemented and enabled by default in all modern IB NICs and Linux OSes.

That offload allows line-rate for IPoIB, something that is not possible
for SW IPoIB.

Thanks

> 
> Thanks
> 
> >
> > Thanks
> >
> > >
> > > > >
> > > > > Thanks!
> > > > >>
> > > > >> I am uncertain about how to proceed at this point and would like to
> > > > >> know your thoughts and opinions on this.
> > > > >>
> > > > >> Thanks,
> > > > >> Kangjing
> > > > >>
> > > > >> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > >> >
> > > > >> > On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> > > > >> > > On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> > > > >> > > >
> > > > >> > > > On Wed, 6 Nov 2024 15:59:10 +0200
> > > > >> > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > > >> > > >
> > > > >> > > > > > Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> > > > >> > > > >
> > > > >> > > > > RDMA core code is drivers/infiniband/core/*.
> > > > >> > > >
> > > > >> > > > Understood. So this is a violation of the no direct access to the
> > > > >> > > > callbacks rule.
> > > > >> > > >
> > > > >> > > > >
> > > > >> > > > > > I would guess it is not, and I would not actually mind sending a patch
> > > > >> > > > > > but I have trouble figuring out the logic behind  commit ecce70cf17d9
> > > > >> > > > > > ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> > > > >> > > > > > ksmbd_rdma_capable_netdev()").
> > > > >> > > > >
> > > > >> > > > > It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> > > > >> > > > > GID, netdev and fabric complexity.
> > > > >> > > >
> > > > >> > > > I'm not familiar enough with either of the subsystems. Based on your
> > > > >> > > > answer my guess is that it ain't outright bugous but still a layering
> > > > >> > > > violation. Copying linux-cifs@vger.kernel.org so that
> > > > >> > > > the smb are aware.
> > > > >> > > Could you please elaborate what the violation is ?
> > > > >> >
> > > > >> > There are many, but the most screaming is that ksmbd has logic to
> > > > >> > differentiate IPoIB devices. These devices are pure netdev devices
> > > > >> > and should be treated like that. ULPs should treat them exactly
> > > > >> > as they treat netdev devices.
> > > > >> >
> > > > >> > > I would also appreciate it if you could suggest to me how to fix this.
> > > > >> > >
> > > > >> > > Thanks.
> > > > >> > > >
> > > > >> > > > Thank you very much for all the explanations!
> > > > >> > > >
> > > > >> > > > Regards,
> > > > >> > > > Halil
> > > > >> > > >
> > > > >>
> > > > >>
> > > > >>
> > > > >> --
> > > > >> Kangjing "Chaser" Huang
> > >
> > >
> > >
> > > --
> > > Kangjing "Chaser" Huang
> 
> 
> 
> -- 
> Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Tom Talpey]

On 1/8/2025 4:31 AM, Leon Romanovsky wrote:
> On Tue, Jan 07, 2025 at 10:51:19PM +0000, Kangjing Huang wrote:
>> On Thu, Dec 19, 2024 at 4:56 PM Leon Romanovsky <leon@kernel.org> wrote:
>>>
>>> On Sat, Dec 14, 2024 at 08:02:14AM +0000, Kangjing Huang wrote:
>>>> On Sat, Dec 14, 2024 at 1:06 AM Leon Romanovsky <leon@kernel.org> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On Sat, Dec 14, 2024, at 04:33, Namjae Jeon wrote:
>>>>>> On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
>>>>>>>
>>>>>>> Hi there,
>>>>>>>
>>>>>>> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
>>>>>>> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
>>>>>>> as mentioned in the thread.
>>>>>>>
>>>>>>> I am working on modifying the patch to take care of the layering
>>>>>>> violation. The original patch was meant to fix an issue with ksmbd,
>>>>>>> where an IPoIB netdev was not recognized as RDMA-capable. The original
>>>>>>> version of the capability evaluation tries to match each netdev to
>>>>>>> ib_device by calling get_netdev in ib verbs. However this only works
>>>>>>> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
>>>>>>> and since with IPoIB it is the other way around (netdev is the upper
>>>>>>> layer of ib_device), get_netdev won't work anymore.
>>>>>>>
>>>>>>> I tried to replicate the behavior of device matching reversely in the
>>>>>>> original version of my patch using GID, which ended up as the layering
>>>>>>> violation. However I am unaware of any exported functions from the
>>>>>>> IPoIB driver that could do the reverse lookup from netdev to the lower
>>>>>>> layer ib_device. Actually it seems that the IPoIB driver does not have
>>>>>>> any exported symbols at all.
>>>>>>>
>>>>>>> It might be that the device matching in reverse just does not make any
>>>>>>> sense and does not need to be done at all. As long as it is an IPoIB
>>>>>>> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
>>>>>>> automatically assume it is RDMA-capable. I am not 100% sure about this
>>>>>>> though.
>>>>>> Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
>>>>>> How about assuming it's RDMA-capable and allowing users to turn
>>>>>> RDMA-capable on/off via sysfs?
>>>> It does make more sense to me at this point to just broadly assume all
>>>> ARPHRD_INFINIBAND types to be RDMA-capable, we just need to make sure
>>>> this assumption indeed holds and figure out to what extent this could
>>>> involve the same layering violation.
>>>>
>>>>>
>>>>> Any attempt to treat ipoib differently from regular netdevice is wrong by definition.
>>>>>
>>>> I would agree that the design direction to treat ipoib as a pure
>>>> regular net_device is the good way to go. But the problem with ksmbd
>>>> and ipoib devices stems from the SMB protocol itself.
>>>>
>>>> In contrast to protocols that focus on certain functionalities like
>>>> nfs, SMB actually tries to manage network interfaces actively in the
>>>> protocol itself: SMB protocol's RDMA support (dubbed SMB Direct) is a
>>>> sub-feature of SMB Multichannel. Multichannel is designed to let
>>>> client and server find multiple data paths automatically (imagine a
>>>> pair of hosts with multiple adapters connected by multiple cables) to
>>>> increase bandwidth. So client can initiate a
>>>> FSCTL_QUERY_NETWORK_INTERFACE_INFO request and server is expected to
>>>> respond with NETWORK_INTERFACE_INFO containing _all_ local network
>>>> interface informations, including their capabilities such as
>>>> RDMA_CAPABLE (for details see ref [MS-SMB2] 3.3.5.15.11) Only upon
>>>> seeing the capability flag would a client attempt to initiate a RDMA
>>>> connection.
>>>>
>>>> Reference: [MS-SMB2](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d.pdf)
>>>>
>>>> TLDR is that the SMB protocol requires the server to enumerate all
>>>> net_devices and indicate their RDMA capability, and
>>>> ksmbd_rdma_capable_netdev() is only used in that process. Given such
>>>> context, I wonder what should be the best way to approach this? Is
>>>> using ARPHRD_INFINIBAND good enough and acceptable in terms of
>>>> layering?
>>>
>>
>>> The thing is that ARPHRD_INFINIBAND indeed represent IPoIB and it is
>>> right check if netdev is IPoIB or not. The layering problem is that
>>> upper layers (ULPs) should use it as regular netdevice.
>>
>> This is good to know. However, since the SMB protocol explicitly calls
>> for enumeration of all network interfaces on the server host,
>> including their RDMA capabilities, I believe this is a sensible
>> exception to the layering rule. Or is there anyway else to do this
>> enumeration from the kernel space?
>>
>> Or we can give up implementing the full spec of the SMB protocol and
>> call for explicit configuration from user space on how to respond to
>> the IOCTL requests in question. Which one looks more sensible to you?
> 
> My preference is to have same IPoIB treatment for all ULPs, including SMB.
> 
> My GUESS is that SMB specification authors didn't take into account HW and
> Linux SW development around IPoIB and weren't aware of IPoIB offload which
> is implemented and enabled by default in all modern IB NICs and Linux OSes.

The SMB3 specification is completely unconcerned with IPoIB and any
other layer-2 or layer-3 implementation details. It merely discusses
an exchange of network interface capabilities such as link speed and
RDMA support. The SMB3 client uses this list to implement multichannel.

I totally agree that inspecting ARPHRD_INFINIBAND is an incorrect method
of building this list. Just because an interface supports IPoIB does not
mean it also exposes RDMA, especially in-kernel. And that ignores any
non-IB transport too of course.

Kangjing, please educate me if I'm confused here, but doesn't the
code in ksmbd_rdma_capable_netdev() look up the ib_device anyway, at
the end of the function?

> 	if (rdma_capable == false) {
> 		struct ib_device *ibdev;
> 
> 		ibdev = ib_device_get_by_netdev(netdev, RDMA_DRIVER_UNKNOWN);
> 		if (ibdev) {
> 			if (rdma_frwr_is_supported(&ibdev->attrs))
> 				rdma_capable = true;
> 			ib_device_put(ibdev);
> 		}
> 	}
> 
> 	return rdma_capable;

So, why is the code concerned at all with ARPHRD_INFINIBAND just a few
lines above? And why does it look in the smb_direct_device_list first?

Tom.

> 
> That offload allows line-rate for IPoIB, something that is not possible
> for SW IPoIB.
> 
> Thanks
> 
>>
>> Thanks
>>
>>>
>>> Thanks
>>>
>>>>
>>>>>>
>>>>>> Thanks!
>>>>>>>
>>>>>>> I am uncertain about how to proceed at this point and would like to
>>>>>>> know your thoughts and opinions on this.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Kangjing
>>>>>>>
>>>>>>> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
>>>>>>>>
>>>>>>>> On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
>>>>>>>>> On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On Wed, 6 Nov 2024 15:59:10 +0200
>>>>>>>>>> Leon Romanovsky <leon@kernel.org> wrote:
>>>>>>>>>>
>>>>>>>>>>>> Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
>>>>>>>>>>>
>>>>>>>>>>> RDMA core code is drivers/infiniband/core/*.
>>>>>>>>>>
>>>>>>>>>> Understood. So this is a violation of the no direct access to the
>>>>>>>>>> callbacks rule.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> I would guess it is not, and I would not actually mind sending a patch
>>>>>>>>>>>> but I have trouble figuring out the logic behind  commit ecce70cf17d9
>>>>>>>>>>>> ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
>>>>>>>>>>>> ksmbd_rdma_capable_netdev()").
>>>>>>>>>>>
>>>>>>>>>>> It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
>>>>>>>>>>> GID, netdev and fabric complexity.
>>>>>>>>>>
>>>>>>>>>> I'm not familiar enough with either of the subsystems. Based on your
>>>>>>>>>> answer my guess is that it ain't outright bugous but still a layering
>>>>>>>>>> violation. Copying linux-cifs@vger.kernel.org so that
>>>>>>>>>> the smb are aware.
>>>>>>>>> Could you please elaborate what the violation is ?
>>>>>>>>
>>>>>>>> There are many, but the most screaming is that ksmbd has logic to
>>>>>>>> differentiate IPoIB devices. These devices are pure netdev devices
>>>>>>>> and should be treated like that. ULPs should treat them exactly
>>>>>>>> as they treat netdev devices.
>>>>>>>>
>>>>>>>>> I would also appreciate it if you could suggest to me how to fix this.
>>>>>>>>>
>>>>>>>>> Thanks.
>>>>>>>>>>
>>>>>>>>>> Thank you very much for all the explanations!
>>>>>>>>>>
>>>>>>>>>> Regards,
>>>>>>>>>> Halil
>>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Kangjing "Chaser" Huang
>>>>
>>>>
>>>>
>>>> --
>>>> Kangjing "Chaser" Huang
>>
>>
>>
>> -- 
>> Kangjing "Chaser" Huang
> 
> 

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Kangjing Huang]

On Wed, Jan 8, 2025 at 5:27 PM Tom Talpey <tom@talpey.com> wrote:
>
> On 1/8/2025 4:31 AM, Leon Romanovsky wrote:
> > On Tue, Jan 07, 2025 at 10:51:19PM +0000, Kangjing Huang wrote:
> >> On Thu, Dec 19, 2024 at 4:56 PM Leon Romanovsky <leon@kernel.org> wrote:
> >>>
> >>> On Sat, Dec 14, 2024 at 08:02:14AM +0000, Kangjing Huang wrote:
> >>>> On Sat, Dec 14, 2024 at 1:06 AM Leon Romanovsky <leon@kernel.org> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On Sat, Dec 14, 2024, at 04:33, Namjae Jeon wrote:
> >>>>>> On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
> >>>>>>>
> >>>>>>> Hi there,
> >>>>>>>
> >>>>>>> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
> >>>>>>> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
> >>>>>>> as mentioned in the thread.
> >>>>>>>
> >>>>>>> I am working on modifying the patch to take care of the layering
> >>>>>>> violation. The original patch was meant to fix an issue with ksmbd,
> >>>>>>> where an IPoIB netdev was not recognized as RDMA-capable. The original
> >>>>>>> version of the capability evaluation tries to match each netdev to
> >>>>>>> ib_device by calling get_netdev in ib verbs. However this only works
> >>>>>>> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
> >>>>>>> and since with IPoIB it is the other way around (netdev is the upper
> >>>>>>> layer of ib_device), get_netdev won't work anymore.
> >>>>>>>
> >>>>>>> I tried to replicate the behavior of device matching reversely in the
> >>>>>>> original version of my patch using GID, which ended up as the layering
> >>>>>>> violation. However I am unaware of any exported functions from the
> >>>>>>> IPoIB driver that could do the reverse lookup from netdev to the lower
> >>>>>>> layer ib_device. Actually it seems that the IPoIB driver does not have
> >>>>>>> any exported symbols at all.
> >>>>>>>
> >>>>>>> It might be that the device matching in reverse just does not make any
> >>>>>>> sense and does not need to be done at all. As long as it is an IPoIB
> >>>>>>> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
> >>>>>>> automatically assume it is RDMA-capable. I am not 100% sure about this
> >>>>>>> though.
> >>>>>> Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
> >>>>>> How about assuming it's RDMA-capable and allowing users to turn
> >>>>>> RDMA-capable on/off via sysfs?
> >>>> It does make more sense to me at this point to just broadly assume all
> >>>> ARPHRD_INFINIBAND types to be RDMA-capable, we just need to make sure
> >>>> this assumption indeed holds and figure out to what extent this could
> >>>> involve the same layering violation.
> >>>>
> >>>>>
> >>>>> Any attempt to treat ipoib differently from regular netdevice is wrong by definition.
> >>>>>
> >>>> I would agree that the design direction to treat ipoib as a pure
> >>>> regular net_device is the good way to go. But the problem with ksmbd
> >>>> and ipoib devices stems from the SMB protocol itself.
> >>>>
> >>>> In contrast to protocols that focus on certain functionalities like
> >>>> nfs, SMB actually tries to manage network interfaces actively in the
> >>>> protocol itself: SMB protocol's RDMA support (dubbed SMB Direct) is a
> >>>> sub-feature of SMB Multichannel. Multichannel is designed to let
> >>>> client and server find multiple data paths automatically (imagine a
> >>>> pair of hosts with multiple adapters connected by multiple cables) to
> >>>> increase bandwidth. So client can initiate a
> >>>> FSCTL_QUERY_NETWORK_INTERFACE_INFO request and server is expected to
> >>>> respond with NETWORK_INTERFACE_INFO containing _all_ local network
> >>>> interface informations, including their capabilities such as
> >>>> RDMA_CAPABLE (for details see ref [MS-SMB2] 3.3.5.15.11) Only upon
> >>>> seeing the capability flag would a client attempt to initiate a RDMA
> >>>> connection.
> >>>>
> >>>> Reference: [MS-SMB2](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d.pdf)
> >>>>
> >>>> TLDR is that the SMB protocol requires the server to enumerate all
> >>>> net_devices and indicate their RDMA capability, and
> >>>> ksmbd_rdma_capable_netdev() is only used in that process. Given such
> >>>> context, I wonder what should be the best way to approach this? Is
> >>>> using ARPHRD_INFINIBAND good enough and acceptable in terms of
> >>>> layering?
> >>>
> >>
> >>> The thing is that ARPHRD_INFINIBAND indeed represent IPoIB and it is
> >>> right check if netdev is IPoIB or not. The layering problem is that
> >>> upper layers (ULPs) should use it as regular netdevice.
> >>
> >> This is good to know. However, since the SMB protocol explicitly calls
> >> for enumeration of all network interfaces on the server host,
> >> including their RDMA capabilities, I believe this is a sensible
> >> exception to the layering rule. Or is there anyway else to do this
> >> enumeration from the kernel space?
> >>
> >> Or we can give up implementing the full spec of the SMB protocol and
> >> call for explicit configuration from user space on how to respond to
> >> the IOCTL requests in question. Which one looks more sensible to you?
> >
> > My preference is to have same IPoIB treatment for all ULPs, including SMB.

If that is the case, then we don't really have many options left but
to expose the advertisement of rdma-capable flags as a configurable
option from the user space (maybe via sysfs). I am not sure if this
will allow the user to mark an interface as RDMA-capable despite its
incapability, or would the capability check be allowed to be made from
the configuration validation site?

> >
> > My GUESS is that SMB specification authors didn't take into account HW and
> > Linux SW development around IPoIB and weren't aware of IPoIB offload which
> > is implemented and enabled by default in all modern IB NICs and Linux OSes.
>
> The SMB3 specification is completely unconcerned with IPoIB and any
> other layer-2 or layer-3 implementation details. It merely discusses
> an exchange of network interface capabilities such as link speed and
> RDMA support. The SMB3 client uses this list to implement multichannel.
>
> I totally agree that inspecting ARPHRD_INFINIBAND is an incorrect method
> of building this list. Just because an interface supports IPoIB does not
> mean it also exposes RDMA, especially in-kernel. And that ignores any
> non-IB transport too of course.

I see, so if I understand this argument correctly, the point is that
even if an IPoIB device *is* guaranteed to have some RDMA support in
the stack (since IPoIB is ULP of an IB device, which always supports
RDMA), it does not necessarily want to expose that capability to its
own ULP in the stacks above, right? However if this is the case, then
what would be the correct way to determine the capability flag from a
ULP?

>
> Kangjing, please educate me if I'm confused here, but doesn't the
> code in ksmbd_rdma_capable_netdev() look up the ib_device anyway, at
> the end of the function?
>
> >       if (rdma_capable == false) {
> >               struct ib_device *ibdev;
> >
> >               ibdev = ib_device_get_by_netdev(netdev, RDMA_DRIVER_UNKNOWN);
> >               if (ibdev) {
> >                       if (rdma_frwr_is_supported(&ibdev->attrs))
> >                               rdma_capable = true;
> >                       ib_device_put(ibdev);
> >               }
> >       }
> >
> >       return rdma_capable;
>
> So, why is the code concerned at all with ARPHRD_INFINIBAND just a few
> lines above?

This check does not work with IPoIB devices. For IPoIB devices
ib_device_get_by_netdev would just return null.

Correct me if I am wrong but I think this API is intended to work with
RDMA/IB transport as ULP of a normal netdev transport (e.g. RoCE,
iWARP). It essentially only performs a lookup of the devices up the
stack, not down the stack, thus not working for IPoIB devices. I feel
this is somehow intended as well since it works inline with the
principle that IPoIB devices should just be treated like a normal
netdev.

This is probably true for the get_netdev verb on ib_device, and the
ib_device_get_netdev API, they are just performing the lookup in
reverse direction.

> And why does it look in the smb_direct_device_list first?

I don't know - this section of code is already here before my patch,
and it confuses me a lot too. It is actually doing the lookup in one
direction and then doing it again in the other direction, probably on
the same mapping. I just left it there during initial patch
submission, just in fear of breaking any strange corner cases.

Thanks,
Kangjing


>
> Tom.
>
> >
> > That offload allows line-rate for IPoIB, something that is not possible
> > for SW IPoIB.
> >
> > Thanks
> >
> >>
> >> Thanks
> >>
> >>>
> >>> Thanks
> >>>
> >>>>
> >>>>>>
> >>>>>> Thanks!
> >>>>>>>
> >>>>>>> I am uncertain about how to proceed at this point and would like to
> >>>>>>> know your thoughts and opinions on this.
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> Kangjing
> >>>>>>>
> >>>>>>> On Fri, Nov 8, 2024 at 5:59 PM Leon Romanovsky <leon@kernel.org> wrote:
> >>>>>>>>
> >>>>>>>> On Fri, Nov 08, 2024 at 08:40:40AM +0900, Namjae Jeon wrote:
> >>>>>>>>> On Thu, Nov 7, 2024 at 9:00 PM Halil Pasic <pasic@linux.ibm.com> wrote:
> >>>>>>>>>>
> >>>>>>>>>> On Wed, 6 Nov 2024 15:59:10 +0200
> >>>>>>>>>> Leon Romanovsky <leon@kernel.org> wrote:
> >>>>>>>>>>
> >>>>>>>>>>>> Does  fs/smb/server/transport_rdma.c qualify as inside of RDMA core code?
> >>>>>>>>>>>
> >>>>>>>>>>> RDMA core code is drivers/infiniband/core/*.
> >>>>>>>>>>
> >>>>>>>>>> Understood. So this is a violation of the no direct access to the
> >>>>>>>>>> callbacks rule.
> >>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>> I would guess it is not, and I would not actually mind sending a patch
> >>>>>>>>>>>> but I have trouble figuring out the logic behind  commit ecce70cf17d9
> >>>>>>>>>>>> ("ksmbd: fix missing RDMA-capable flag for IPoIB device in
> >>>>>>>>>>>> ksmbd_rdma_capable_netdev()").
> >>>>>>>>>>>
> >>>>>>>>>>> It is strange version of RDMA-CM. All other ULPs use RDMA-CM to avoid
> >>>>>>>>>>> GID, netdev and fabric complexity.
> >>>>>>>>>>
> >>>>>>>>>> I'm not familiar enough with either of the subsystems. Based on your
> >>>>>>>>>> answer my guess is that it ain't outright bugous but still a layering
> >>>>>>>>>> violation. Copying linux-cifs@vger.kernel.org so that
> >>>>>>>>>> the smb are aware.
> >>>>>>>>> Could you please elaborate what the violation is ?
> >>>>>>>>
> >>>>>>>> There are many, but the most screaming is that ksmbd has logic to
> >>>>>>>> differentiate IPoIB devices. These devices are pure netdev devices
> >>>>>>>> and should be treated like that. ULPs should treat them exactly
> >>>>>>>> as they treat netdev devices.
> >>>>>>>>
> >>>>>>>>> I would also appreciate it if you could suggest to me how to fix this.
> >>>>>>>>>
> >>>>>>>>> Thanks.
> >>>>>>>>>>
> >>>>>>>>>> Thank you very much for all the explanations!
> >>>>>>>>>>
> >>>>>>>>>> Regards,
> >>>>>>>>>> Halil
> >>>>>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> --
> >>>>>>> Kangjing "Chaser" Huang
> >>>>
> >>>>
> >>>>
> >>>> --
> >>>> Kangjing "Chaser" Huang
> >>
> >>
> >>
> >> --
> >> Kangjing "Chaser" Huang
> >
> >
>


--
Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Leon Romanovsky]

On Wed, Jan 08, 2025 at 10:40:22PM +0000, Kangjing Huang wrote:
> On Wed, Jan 8, 2025 at 5:27 PM Tom Talpey <tom@talpey.com> wrote:
> >
> > On 1/8/2025 4:31 AM, Leon Romanovsky wrote:
> > > On Tue, Jan 07, 2025 at 10:51:19PM +0000, Kangjing Huang wrote:
> > >> On Thu, Dec 19, 2024 at 4:56 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >>>
> > >>> On Sat, Dec 14, 2024 at 08:02:14AM +0000, Kangjing Huang wrote:
> > >>>> On Sat, Dec 14, 2024 at 1:06 AM Leon Romanovsky <leon@kernel.org> wrote:
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>> On Sat, Dec 14, 2024, at 04:33, Namjae Jeon wrote:
> > >>>>>> On Fri, Dec 13, 2024 at 8:07 PM Kangjing Huang <huangkangjing@gmail.com> wrote:
> > >>>>>>>
> > >>>>>>> Hi there,
> > >>>>>>>
> > >>>>>>> I am the original author of commit ecce70cf17d9 ("ksmbd: fix missing
> > >>>>>>> RDMA-capable flag for IPoIB device in ksmbd_rdma_capable_netdev()"),
> > >>>>>>> as mentioned in the thread.
> > >>>>>>>
> > >>>>>>> I am working on modifying the patch to take care of the layering
> > >>>>>>> violation. The original patch was meant to fix an issue with ksmbd,
> > >>>>>>> where an IPoIB netdev was not recognized as RDMA-capable. The original
> > >>>>>>> version of the capability evaluation tries to match each netdev to
> > >>>>>>> ib_device by calling get_netdev in ib verbs. However this only works
> > >>>>>>> in cases where the ib_device is the upper layer of netdev (e.g. RoCE),
> > >>>>>>> and since with IPoIB it is the other way around (netdev is the upper
> > >>>>>>> layer of ib_device), get_netdev won't work anymore.
> > >>>>>>>
> > >>>>>>> I tried to replicate the behavior of device matching reversely in the
> > >>>>>>> original version of my patch using GID, which ended up as the layering
> > >>>>>>> violation. However I am unaware of any exported functions from the
> > >>>>>>> IPoIB driver that could do the reverse lookup from netdev to the lower
> > >>>>>>> layer ib_device. Actually it seems that the IPoIB driver does not have
> > >>>>>>> any exported symbols at all.
> > >>>>>>>
> > >>>>>>> It might be that the device matching in reverse just does not make any
> > >>>>>>> sense and does not need to be done at all. As long as it is an IPoIB
> > >>>>>>> device (netdev->type == ARPHRD_INFINIBAND) it might be ok to just
> > >>>>>>> automatically assume it is RDMA-capable. I am not 100% sure about this
> > >>>>>>> though.
> > >>>>>> Why can't we assume RDMA-capable if it's ARPHRD_INFINIBAND type?
> > >>>>>> How about assuming it's RDMA-capable and allowing users to turn
> > >>>>>> RDMA-capable on/off via sysfs?
> > >>>> It does make more sense to me at this point to just broadly assume all
> > >>>> ARPHRD_INFINIBAND types to be RDMA-capable, we just need to make sure
> > >>>> this assumption indeed holds and figure out to what extent this could
> > >>>> involve the same layering violation.
> > >>>>
> > >>>>>
> > >>>>> Any attempt to treat ipoib differently from regular netdevice is wrong by definition.
> > >>>>>
> > >>>> I would agree that the design direction to treat ipoib as a pure
> > >>>> regular net_device is the good way to go. But the problem with ksmbd
> > >>>> and ipoib devices stems from the SMB protocol itself.
> > >>>>
> > >>>> In contrast to protocols that focus on certain functionalities like
> > >>>> nfs, SMB actually tries to manage network interfaces actively in the
> > >>>> protocol itself: SMB protocol's RDMA support (dubbed SMB Direct) is a
> > >>>> sub-feature of SMB Multichannel. Multichannel is designed to let
> > >>>> client and server find multiple data paths automatically (imagine a
> > >>>> pair of hosts with multiple adapters connected by multiple cables) to
> > >>>> increase bandwidth. So client can initiate a
> > >>>> FSCTL_QUERY_NETWORK_INTERFACE_INFO request and server is expected to
> > >>>> respond with NETWORK_INTERFACE_INFO containing _all_ local network
> > >>>> interface informations, including their capabilities such as
> > >>>> RDMA_CAPABLE (for details see ref [MS-SMB2] 3.3.5.15.11) Only upon
> > >>>> seeing the capability flag would a client attempt to initiate a RDMA
> > >>>> connection.
> > >>>>
> > >>>> Reference: [MS-SMB2](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d.pdf)
> > >>>>
> > >>>> TLDR is that the SMB protocol requires the server to enumerate all
> > >>>> net_devices and indicate their RDMA capability, and
> > >>>> ksmbd_rdma_capable_netdev() is only used in that process. Given such
> > >>>> context, I wonder what should be the best way to approach this? Is
> > >>>> using ARPHRD_INFINIBAND good enough and acceptable in terms of
> > >>>> layering?
> > >>>
> > >>
> > >>> The thing is that ARPHRD_INFINIBAND indeed represent IPoIB and it is
> > >>> right check if netdev is IPoIB or not. The layering problem is that
> > >>> upper layers (ULPs) should use it as regular netdevice.
> > >>
> > >> This is good to know. However, since the SMB protocol explicitly calls
> > >> for enumeration of all network interfaces on the server host,
> > >> including their RDMA capabilities, I believe this is a sensible
> > >> exception to the layering rule. Or is there anyway else to do this
> > >> enumeration from the kernel space?
> > >>
> > >> Or we can give up implementing the full spec of the SMB protocol and
> > >> call for explicit configuration from user space on how to respond to
> > >> the IOCTL requests in question. Which one looks more sensible to you?
> > >
> > > My preference is to have same IPoIB treatment for all ULPs, including SMB.
> 
> If that is the case, then we don't really have many options left but
> to expose the advertisement of rdma-capable flags as a configurable
> option from the user space (maybe via sysfs). I am not sure if this
> will allow the user to mark an interface as RDMA-capable despite its
> incapability, or would the capability check be allowed to be made from
> the configuration validation site?

Except SMB spec, why do you need to provide "RDMA-capable" information?
Is it must? What will it give to the users? Why can't you treat IPoIB
like any other netdevice?

> 
> > >
> > > My GUESS is that SMB specification authors didn't take into account HW and
> > > Linux SW development around IPoIB and weren't aware of IPoIB offload which
> > > is implemented and enabled by default in all modern IB NICs and Linux OSes.
> >
> > The SMB3 specification is completely unconcerned with IPoIB and any
> > other layer-2 or layer-3 implementation details. It merely discusses
> > an exchange of network interface capabilities such as link speed and
> > RDMA support. The SMB3 client uses this list to implement multichannel.
> >
> > I totally agree that inspecting ARPHRD_INFINIBAND is an incorrect method
> > of building this list. Just because an interface supports IPoIB does not
> > mean it also exposes RDMA, especially in-kernel. And that ignores any
> > non-IB transport too of course.
> 
> I see, so if I understand this argument correctly, the point is that
> even if an IPoIB device *is* guaranteed to have some RDMA support in
> the stack (since IPoIB is ULP of an IB device, which always supports
> RDMA), it does not necessarily want to expose that capability to its
> own ULP in the stacks above, right? However if this is the case, then
> what would be the correct way to determine the capability flag from a
> ULP?

Let's start with an answer to more simple question: "why do you need
this capability flag?"

> 
> >
> > Kangjing, please educate me if I'm confused here, but doesn't the
> > code in ksmbd_rdma_capable_netdev() look up the ib_device anyway, at
> > the end of the function?
> >
> > >       if (rdma_capable == false) {
> > >               struct ib_device *ibdev;
> > >
> > >               ibdev = ib_device_get_by_netdev(netdev, RDMA_DRIVER_UNKNOWN);
> > >               if (ibdev) {
> > >                       if (rdma_frwr_is_supported(&ibdev->attrs))
> > >                               rdma_capable = true;
> > >                       ib_device_put(ibdev);
> > >               }
> > >       }
> > >
> > >       return rdma_capable;
> >
> > So, why is the code concerned at all with ARPHRD_INFINIBAND just a few
> > lines above?
> 
> This check does not work with IPoIB devices. For IPoIB devices
> ib_device_get_by_netdev would just return null.
> 
> Correct me if I am wrong but I think this API is intended to work with
> RDMA/IB transport as ULP of a normal netdev transport (e.g. RoCE,
> iWARP). It essentially only performs a lookup of the devices up the
> stack, not down the stack, thus not working for IPoIB devices. I feel
> this is somehow intended as well since it works inline with the
> principle that IPoIB devices should just be treated like a normal
> netdev.
> 
> This is probably true for the get_netdev verb on ib_device, and the
> ib_device_get_netdev API, they are just performing the lookup in
> reverse direction.

Yes, ib_device_get_by_netdev and get_netdev are intended to perform
lookup of ib device based on underlying netdev, but in IPoIB case you
are interested in ib device based on upper netdev.

So this leads to another question, if user didn't ask to connect SMB to
IB device and chose netdevice (IPoIB) instead, why are you still forcing
him to take IB path? If it is not user's decision and you are choosing
devices from the list, you already have in your list the IB device which
is connected to IPoIB.

Thanks

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Christoph Hellwig]

On Tue, Jan 07, 2025 at 10:51:19PM +0000, Kangjing Huang wrote:
> This is good to know. However, since the SMB protocol explicitly calls
> for enumeration of all network interfaces on the server host,
> including their RDMA capabilities, I believe this is a sensible
> exception to the layering rule. Or is there anyway else to do this
> enumeration from the kernel space?

No, it's not a sensible exception.  It's a massive data leak and a
completely idiotic protocol feasture Linux should not support.  If the
protocol requires a lsit of network interfaces, a Linux server should
require explicitly require that list to be configured and not expose
private information to the untrusted network.

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Kangjing Huang]

On Thu, Jan 9, 2025 at 8:02 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, Jan 07, 2025 at 10:51:19PM +0000, Kangjing Huang wrote:
> > This is good to know. However, since the SMB protocol explicitly calls
> > for enumeration of all network interfaces on the server host,
> > including their RDMA capabilities, I believe this is a sensible
> > exception to the layering rule. Or is there anyway else to do this
> > enumeration from the kernel space?
>
> No, it's not a sensible exception.  It's a massive data leak and a
> completely idiotic protocol feasture Linux should not support.  If the
> protocol requires a lsit of network interfaces, a Linux server should
> require explicitly require that list to be configured and not expose
> private information to the untrusted network.
>

I see the point. I wasn't thinking about it from the security
perspective too much and would agree with you on this argument about
data leaks.

The protocol itself is not necessarily unsecured, since in the spec it
calls for enumeration in an "implementation-specific manner"[1]. I
would interpret that as the implementation could enumerate interfaces
as it sees fit to its functional and security perspectives. The spec
only requires the server to always return a list of client-usable
interfaces, that does not need to always be the full interface list.

That being said, from ksmbd's implementation perspective, this
definitely needs to be something to be explicitly configured.

ref: [1](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d-240729.pdf)
sec 3.3.5.15.11

On Thu, Jan 9, 2025 at 7:59 AM Leon Romanovsky <leon@kernel.org> wrote:
...

> Except SMB spec, why do you need to provide "RDMA-capable" information?
> Is it must? What will it give to the users? Why can't you treat IPoIB
> like any other netdevice?

...

> Let's start with an answer to more simple question: "why do you need
> this capability flag?"

The short answer is: a Windows client requires this flag to be present
to actually utilize RDMA.

Long answer:

Although SMB-Direct (Microsoft speak for SMB over RDMA) protocol
technically is just wrapping SMB packets in RDMA transport, allowing
for a SMB connection to be completely handshaked and established in
RDMA context, without any need for transferring ip packets. This kind
of connection seems to be never happening for the actual client
implementation in Windows.

Under Windows, SMB Direct is only enabled when SMB multichannel is
enabled. And there is no way for the user to specify if they want to
connect to the IPoIB endpoint or RDMA endpoint - the client simply
connects to the ip interface first, uses the aforementioned IOCTL
request to query the RDMA capabilities for all interfaces, and upon
finding usable RDMA-capable interfaces, opens preferred data channels
on RDMA transport to carry the actual data.

As far as I know there is no way from a Windows user's perspective to
modify this behavior and upon disabling SMB multichannel, SMB will
just disable RDMA support and transfer everything over IP. The RDMA
transport is only "automatically upgraded to", no explicit
configurations are available whatsoever.

So if ksmbd does not send out capability flags to indicate the Windows
client to initiate RDMA transport as a side channel, the client will
just keep working with connections on the ip stack and the user can
never utilize RDMA.

To be clear: I would strongly agree that the Windows client's behavior
here is pretty stupid and is just outright dumb design. In configuring
my two-workstation setup utilizing this protocol, the no-config design
of the Windows side has created far more problems than it has solved.
It also somehow brought me here submitting my first kernel patch.
However if we want any interoperability of ksmbd with Windows clients
on the RDMA front, we would need to implement the IOCTL response to
some degree and send out the RDMA capability flags.

My apologies for not making this context earlier, I was too
hyper-focused on the code itself to fall into the classic XY problem
trap.

Disclaimer: all my knowledge and research about windows client is
limited to the edition I have access to (Windows 10 Pro Workstation),
I have no idea if Windows server edition's client is different.

...

>
> Yes, ib_device_get_by_netdev and get_netdev are intended to perform
> lookup of ib device based on underlying netdev, but in IPoIB case you
> are interested in ib device based on upper netdev.
>
> So this leads to another question, if user didn't ask to connect SMB to
> IB device and chose netdevice (IPoIB) instead, why are you still forcing
> him to take IB path? If it is not user's decision and you are choosing
> devices from the list, you already have in your list the IB device which
> is connected to IPoIB.

This is related to the long answer to the first question. Basically
using a Windows client the user cannot make decisions to use RDMA or
not, it is not the decision of the server, either - the client will
enumerate the interfaces and make decisions for the user. And due to
the presence of SMB multichannel, the ip interface receiving the
interface enumeration request is not even guaranteed to be the
connected IPoIB interface - the multichannel protocol is designed to
work with service discovery, where it is totally possible that the
initial connection will be established on another data path, before
the client discovers another better path potentially with RDMA
capability. The only way for ksmbd to be fully compatible with these
is to implement the interface enumeration to some degree. Although I
am indeed convinced that this needs to be explicitly configured to
avoid any security risks.

Thanks


>
> Thanks




--
Kangjing "Chaser" Huang

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Tom Talpey]

On 1/9/2025 5:43 AM, Kangjing Huang wrote:
> On Thu, Jan 9, 2025 at 8:02 AM Christoph Hellwig <hch@infradead.org> wrote:
>>
>> On Tue, Jan 07, 2025 at 10:51:19PM +0000, Kangjing Huang wrote:
>>> This is good to know. However, since the SMB protocol explicitly calls
>>> for enumeration of all network interfaces on the server host,
>>> including their RDMA capabilities, I believe this is a sensible
>>> exception to the layering rule. Or is there anyway else to do this
>>> enumeration from the kernel space?
>>
>> No, it's not a sensible exception.  It's a massive data leak and a
>> completely idiotic protocol feasture Linux should not support.  If the
>> protocol requires a lsit of network interfaces, a Linux server should
>> require explicitly require that list to be configured and not expose
>> private information to the untrusted network.
>>
> 
> I see the point. I wasn't thinking about it from the security
> perspective too much and would agree with you on this argument about
> data leaks.
> 
> The protocol itself is not necessarily unsecured, since in the spec it
> calls for enumeration in an "implementation-specific manner"[1]. I
> would interpret that as the implementation could enumerate interfaces
> as it sees fit to its functional and security perspectives. The spec
> only requires the server to always return a list of client-usable
> interfaces, that does not need to always be the full interface list.

Absolutely. Windows has a number of filters on both sides to decide
which interfaces to advertise (server) and which to actually connect
to (client). For example, back-end cluster interfaces are never
exposed via QUERY_NETWORK_INTERFACES.

The ksmbd.conf file already has the "interfaces=" stanza, perhaps
you/we should simply consider using that list as the base. This
effectively would force the "bind interfaces only" flag to true
for RDMA adapters however. Making system-specific lists like these
explicitly opt-in is rarely popular.

Another alternative is to use the rdma_cm, which is the actual thing
controlling the listeners. When it begins listening on an adapter,
perhaps there's a callout that ksmbd can monitor to enumerate the
endpoints that will respond to 445/5445.

Tom.

> 
> That being said, from ksmbd's implementation perspective, this
> definitely needs to be something to be explicitly configured.
> 
> ref: [1](https://winprotocoldoc.z19.web.core.windows.net/MS-SMB2/%5bMS-SMB2%5d-240729.pdf)
> sec 3.3.5.15.11
> 
> On Thu, Jan 9, 2025 at 7:59 AM Leon Romanovsky <leon@kernel.org> wrote:
> ...
> 
>> Except SMB spec, why do you need to provide "RDMA-capable" information?
>> Is it must? What will it give to the users? Why can't you treat IPoIB
>> like any other netdevice?
> 
> ...
> 
>> Let's start with an answer to more simple question: "why do you need
>> this capability flag?"
> 
> The short answer is: a Windows client requires this flag to be present
> to actually utilize RDMA.
> 
> Long answer:
> 
> Although SMB-Direct (Microsoft speak for SMB over RDMA) protocol
> technically is just wrapping SMB packets in RDMA transport, allowing
> for a SMB connection to be completely handshaked and established in
> RDMA context, without any need for transferring ip packets. This kind
> of connection seems to be never happening for the actual client
> implementation in Windows.
> 
> Under Windows, SMB Direct is only enabled when SMB multichannel is
> enabled. And there is no way for the user to specify if they want to
> connect to the IPoIB endpoint or RDMA endpoint - the client simply
> connects to the ip interface first, uses the aforementioned IOCTL
> request to query the RDMA capabilities for all interfaces, and upon
> finding usable RDMA-capable interfaces, opens preferred data channels
> on RDMA transport to carry the actual data.
> 
> As far as I know there is no way from a Windows user's perspective to
> modify this behavior and upon disabling SMB multichannel, SMB will
> just disable RDMA support and transfer everything over IP. The RDMA
> transport is only "automatically upgraded to", no explicit
> configurations are available whatsoever.
> 
> So if ksmbd does not send out capability flags to indicate the Windows
> client to initiate RDMA transport as a side channel, the client will
> just keep working with connections on the ip stack and the user can
> never utilize RDMA.
> 
> To be clear: I would strongly agree that the Windows client's behavior
> here is pretty stupid and is just outright dumb design. In configuring
> my two-workstation setup utilizing this protocol, the no-config design
> of the Windows side has created far more problems than it has solved.
> It also somehow brought me here submitting my first kernel patch.
> However if we want any interoperability of ksmbd with Windows clients
> on the RDMA front, we would need to implement the IOCTL response to
> some degree and send out the RDMA capability flags.
> 
> My apologies for not making this context earlier, I was too
> hyper-focused on the code itself to fall into the classic XY problem
> trap.
> 
> Disclaimer: all my knowledge and research about windows client is
> limited to the edition I have access to (Windows 10 Pro Workstation),
> I have no idea if Windows server edition's client is different.
> 
> ...
> 
>>
>> Yes, ib_device_get_by_netdev and get_netdev are intended to perform
>> lookup of ib device based on underlying netdev, but in IPoIB case you
>> are interested in ib device based on upper netdev.
>>
>> So this leads to another question, if user didn't ask to connect SMB to
>> IB device and chose netdevice (IPoIB) instead, why are you still forcing
>> him to take IB path? If it is not user's decision and you are choosing
>> devices from the list, you already have in your list the IB device which
>> is connected to IPoIB.
> 
> This is related to the long answer to the first question. Basically
> using a Windows client the user cannot make decisions to use RDMA or
> not, it is not the decision of the server, either - the client will
> enumerate the interfaces and make decisions for the user. And due to
> the presence of SMB multichannel, the ip interface receiving the
> interface enumeration request is not even guaranteed to be the
> connected IPoIB interface - the multichannel protocol is designed to
> work with service discovery, where it is totally possible that the
> initial connection will be established on another data path, before
> the client discovers another better path potentially with RDMA
> capability. The only way for ksmbd to be fully compatible with these
> is to implement the interface enumeration to some degree. Although I
> am indeed convinced that this needs to be explicitly configured to
> avoid any security risks.
> 
> Thanks
> 
> 
>>
>> Thanks
> 
> 
> 
> 
> --
> Kangjing "Chaser" Huang
> 
> 

Re: [PATCH net] net/smc: Fix lookup of netdev by using ib_device_get_netdev()

[Christoph Hellwig]

On Thu, Jan 09, 2025 at 12:49:16PM -0500, Tom Talpey wrote:
> Absolutely. Windows has a number of filters on both sides to decide
> which interfaces to advertise (server) and which to actually connect
> to (client). For example, back-end cluster interfaces are never
> exposed via QUERY_NETWORK_INTERFACES.
> 
> The ksmbd.conf file already has the "interfaces=" stanza, perhaps
> you/we should simply consider using that list as the base. This
> effectively would force the "bind interfaces only" flag to true
> for RDMA adapters however. Making system-specific lists like these
> explicitly opt-in is rarely popular.

Filter still seems like the wrong approach vs an explicitly configured
list.  That's what for example nvme over fabrics does for discovery.