* Possible bug in ipoib_reap_dead_ahs in datagram mode
@ 2022-05-04 19:15 Ryan Stone
2022-05-24 7:07 ` Leon Romanovsky
0 siblings, 1 reply; 6+ messages in thread
From: Ryan Stone @ 2022-05-04 19:15 UTC (permalink / raw)
To: linux-rdma
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Possible bug in ipoib_reap_dead_ahs in datagram mode
2022-05-04 19:15 Possible bug in ipoib_reap_dead_ahs in datagram mode Ryan Stone
@ 2022-05-24 7:07 ` Leon Romanovsky
2022-05-24 13:33 ` Ryan Stone
0 siblings, 1 reply; 6+ messages in thread
From: Leon Romanovsky @ 2022-05-24 7:07 UTC (permalink / raw)
To: Ryan Stone; +Cc: linux-rdma
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Possible bug in ipoib_reap_dead_ahs in datagram mode
2022-05-24 7:07 ` Leon Romanovsky
@ 2022-05-24 13:33 ` Ryan Stone
2022-05-24 19:08 ` Leon Romanovsky
0 siblings, 1 reply; 6+ messages in thread
From: Ryan Stone @ 2022-05-24 13:33 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: linux-rdma
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Possible bug in ipoib_reap_dead_ahs in datagram mode
2022-05-24 13:33 ` Ryan Stone
@ 2022-05-24 19:08 ` Leon Romanovsky
2022-05-24 19:18 ` Ryan Stone
0 siblings, 1 reply; 6+ messages in thread
From: Leon Romanovsky @ 2022-05-24 19:08 UTC (permalink / raw)
To: Ryan Stone; +Cc: linux-rdma
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Possible bug in ipoib_reap_dead_ahs in datagram mode
2022-05-24 19:08 ` Leon Romanovsky
@ 2022-05-24 19:18 ` Ryan Stone
2022-05-25 6:10 ` Leon Romanovsky
0 siblings, 1 reply; 6+ messages in thread
From: Ryan Stone @ 2022-05-24 19:18 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: linux-rdma
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Possible bug in ipoib_reap_dead_ahs in datagram mode
2022-05-24 19:18 ` Ryan Stone
@ 2022-05-25 6:10 ` Leon Romanovsky
0 siblings, 0 replies; 6+ messages in thread
From: Leon Romanovsky @ 2022-05-25 6:10 UTC (permalink / raw)
To: Ryan Stone; +Cc: linux-rdma
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-25 6:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-04 19:15 Possible bug in ipoib_reap_dead_ahs in datagram mode Ryan Stone
2022-05-24 7:07 ` Leon Romanovsky
2022-05-24 13:33 ` Ryan Stone
2022-05-24 19:08 ` Leon Romanovsky
2022-05-24 19:18 ` Ryan Stone
2022-05-25 6:10 ` Leon Romanovsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.