nvme-tcp: invalid h2cdata pdus in response to r2t

nvme-tcp: invalid h2cdata pdus in response to r2t

[Jonathan Nicklin]

As recently as 5.16.10, when a target issues an r2t, the host driver
responds with an h2c pdu of whatever length was requested. If the
requested r2t length exceeds MAXH2CDATA, the response causes a fatal
transport error.

In reviewing the code, the driver does not appear to generate multiple
h2c pdus in response to an R2T. We're a bit confused because commit
1d3ef9c3a39e seems to indicate that the code behaves otherwise.

I can see there was some discussion and work on this back in November.
Is there any plan to incorporate the fix? If so, is this likely to
make its way into LTS kernels?

Thanks,
-Jonathan

Re: nvme-tcp: invalid h2cdata pdus in response to r2t

[Sagi Grimberg]

> As recently as 5.16.10, when a target issues an r2t, the host driver
> responds with an h2c pdu of whatever length was requested. If the
> requested r2t length exceeds MAXH2CDATA, the response causes a fatal
> transport error.
> 
> In reviewing the code, the driver does not appear to generate multiple
> h2c pdus in response to an R2T. We're a bit confused because commit
> 1d3ef9c3a39e seems to indicate that the code behaves otherwise.

Yes

> 
> I can see there was some discussion and work on this back in November.
> Is there any plan to incorporate the fix? If so, is this likely to
> make its way into LTS kernels?

We'll need to see if it applies cleanly... If you have specific desire
you can send a backport to a specific LTS to stable kernels.

Re: nvme-tcp: invalid h2cdata pdus in response to r2t

[Jonathan Nicklin]

Good news. The patch applies cleanly with offsets. And, I can confirm
that the host driver issues multiple data h2c pdus correctly. The
various combinations of digest options work as well.

Of course, there are options for target implementations to avoid the
need to send r2ts, but in the interest of correctness and
interoperability, it seems valuable to fix this bug. Whether or not to
shepherd this through to other kernels is a different question...

On Thu, Feb 24, 2022 at 4:22 AM Sagi Grimberg <sagi@grimberg.me> wrote:
>
>
> > As recently as 5.16.10, when a target issues an r2t, the host driver
> > responds with an h2c pdu of whatever length was requested. If the
> > requested r2t length exceeds MAXH2CDATA, the response causes a fatal
> > transport error.
> >
> > In reviewing the code, the driver does not appear to generate multiple
> > h2c pdus in response to an R2T. We're a bit confused because commit
> > 1d3ef9c3a39e seems to indicate that the code behaves otherwise.
>
> Yes
>
> >
> > I can see there was some discussion and work on this back in November.
> > Is there any plan to incorporate the fix? If so, is this likely to
> > make its way into LTS kernels?
>
> We'll need to see if it applies cleanly... If you have specific desire
> you can send a backport to a specific LTS to stable kernels.

Re: nvme-tcp: invalid h2cdata pdus in response to r2t

[Sagi Grimberg]

> Good news. The patch applies cleanly with offsets. And, I can confirm
> that the host driver issues multiple data h2c pdus correctly. The
> various combinations of digest options work as well.
> 
> Of course, there are options for target implementations to avoid the
> need to send r2ts, but in the interest of correctness and
> interoperability, it seems valuable to fix this bug. Whether or not to
> shepherd this through to other kernels is a different question...

Did you happen to look if this patch was picked up by stable?

Re: nvme-tcp: invalid h2cdata pdus in response to r2t

[Jonathan Nicklin]

Yes. It appears that the patch came in with 5.16.15.


On Sun, Mar 13, 2022 at 5:28 PM Sagi Grimberg <sagi@grimberg.me> wrote:
>
>
>
> > Good news. The patch applies cleanly with offsets. And, I can confirm
> > that the host driver issues multiple data h2c pdus correctly. The
> > various combinations of digest options work as well.
> >
> > Of course, there are options for target implementations to avoid the
> > need to send r2ts, but in the interest of correctness and
> > interoperability, it seems valuable to fix this bug. Whether or not to
> > shepherd this through to other kernels is a different question...
>
> Did you happen to look if this patch was picked up by stable?