Possible bug in ipoib_reap_dead_ahs in datagram mode

Possible bug in ipoib_reap_dead_ahs in datagram mode

[Ryan Stone]

I was reading through the IPoIB code and I think that I see a bug that
affects ipoib_reap_dead_ahs() when using datagram mode.

When sending a packet, if we aren't using the CM (which I assume means
that we are using datagram mode), we fall into the following case:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/ulp/ipoib/ipoib_main.c#n1163

The AH for our neighbour has its last_send field set to the return
value from the RDMA driver's send function

If I look at how this is used in ipoib_reap_dead_ahs(), it compares
last_send to the current tail of the completion(?) queue.  I believe
that this is intended to check that the last outstanding WQ entry that
references the AH has completed.

However, if I look at the actual implementation in mlx5, the send
function always returns NETDEV_TX_OK:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c#n635

If my understanding of all of this is correct, this could lead to a
premature freeing of an AH and a use-after-free bug

Re: Possible bug in ipoib_reap_dead_ahs in datagram mode

[Leon Romanovsky]

On Wed, May 04, 2022 at 03:15:13PM -0400, Ryan Stone wrote:
> I was reading through the IPoIB code and I think that I see a bug that
> affects ipoib_reap_dead_ahs() when using datagram mode.
> 
> When sending a packet, if we aren't using the CM (which I assume means
> that we are using datagram mode), we fall into the following case:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/ulp/ipoib/ipoib_main.c#n1163
> 
> The AH for our neighbour has its last_send field set to the return
> value from the RDMA driver's send function
> 
> If I look at how this is used in ipoib_reap_dead_ahs(), it compares
> last_send to the current tail of the completion(?) queue.  I believe
> that this is intended to check that the last outstanding WQ entry that
> references the AH has completed.
> 
> However, if I look at the actual implementation in mlx5, the send
> function always returns NETDEV_TX_OK:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c#n635
> 
> If my understanding of all of this is correct, this could lead to a
> premature freeing of an AH and a use-after-free bug

IPoIB in mlx5 is HW offloaded version of ulp/ipoib one. AFAIK, it doesn't
change "tx_tail" and we won't enter into this if (...).

Thanks

Re: Possible bug in ipoib_reap_dead_ahs in datagram mode

[Ryan Stone]

On Tue, May 24, 2022 at 3:07 AM Leon Romanovsky <leon@kernel.org> wrote:
> IPoIB in mlx5 is HW offloaded version of ulp/ipoib one. AFAIK, it doesn't
> change "tx_tail" and we won't enter into this if (...).
>
> Thanks

I don't quite follow this response.  Is this the if statement that you
mean that we won't fall into?

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/ulp/ipoib/ipoib_ib.c#n682

Re: Possible bug in ipoib_reap_dead_ahs in datagram mode

[Leon Romanovsky]

On Tue, May 24, 2022 at 09:33:52AM -0400, Ryan Stone wrote:
> On Tue, May 24, 2022 at 3:07 AM Leon Romanovsky <leon@kernel.org> wrote:
> > IPoIB in mlx5 is HW offloaded version of ulp/ipoib one. AFAIK, it doesn't
> > change "tx_tail" and we won't enter into this if (...).
> >
> > Thanks
> 
> I don't quite follow this response.  Is this the if statement that you
> mean that we won't fall into?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/ulp/ipoib/ipoib_ib.c#n682

Yes, I think so, maybe wrong here.

Re: Possible bug in ipoib_reap_dead_ahs in datagram mode

[Ryan Stone]

I believe that if we never enter that if statement, then we will leak
the entries that are supposed to be cleaned up.  That's better than
the use-after-free that I feared but still not good.

On Tue, May 24, 2022 at 3:08 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Tue, May 24, 2022 at 09:33:52AM -0400, Ryan Stone wrote:
> > On Tue, May 24, 2022 at 3:07 AM Leon Romanovsky <leon@kernel.org> wrote:
> > > IPoIB in mlx5 is HW offloaded version of ulp/ipoib one. AFAIK, it doesn't
> > > change "tx_tail" and we won't enter into this if (...).
> > >
> > > Thanks
> >
> > I don't quite follow this response.  Is this the if statement that you
> > mean that we won't fall into?
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/ulp/ipoib/ipoib_ib.c#n682
>
> Yes, I think so, maybe wrong here.

Re: Possible bug in ipoib_reap_dead_ahs in datagram mode

[Leon Romanovsky]

On Tue, May 24, 2022 at 03:18:53PM -0400, Ryan Stone wrote:
> I believe that if we never enter that if statement, then we will leak
> the entries that are supposed to be cleaned up.  That's better than
> the use-after-free that I feared but still not good.

I don't know, we are running enhanced IPoIB all the time and I never got
any reports about kmemleaks/KASAN in that area.

Thanks

> 
> On Tue, May 24, 2022 at 3:08 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Tue, May 24, 2022 at 09:33:52AM -0400, Ryan Stone wrote:
> > > On Tue, May 24, 2022 at 3:07 AM Leon Romanovsky <leon@kernel.org> wrote:
> > > > IPoIB in mlx5 is HW offloaded version of ulp/ipoib one. AFAIK, it doesn't
> > > > change "tx_tail" and we won't enter into this if (...).
> > > >
> > > > Thanks
> > >
> > > I don't quite follow this response.  Is this the if statement that you
> > > mean that we won't fall into?
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/ulp/ipoib/ipoib_ib.c#n682
> >
> > Yes, I think so, maybe wrong here.