Re: [PATCH v2] usb: gadget: ncm: Avoid dropping datagrams of properly parsed NTBs

[Maciej Żenczykowski]

On Wed, Jan 31, 2024 at 9:14 AM Maciej Żenczykowski <maze@xxxxxxxxxx> wrote:
>
> On Wed, Jan 31, 2024 at 9:03 AM Maciej Żenczykowski <maze@xxxxxxxxxx> wrote:
> >
> > On Wed, Jan 31, 2024 at 7:03 AM Krishna Kurapati
> > <quic_kriskura@xxxxxxxxxxx> wrote:
> > >
> > > It is observed sometimes when tethering is used over NCM with Windows 11
> > > as host, at some instances, the gadget_giveback has one byte appended at
> > > the end of a proper NTB. When the NTB is parsed, unwrap call looks for
> > > any leftover bytes in SKB provided by u_ether and if there are any pending
> > > bytes, it treats them as a separate NTB and parses it. But in case the
> > > second NTB (as per unwrap call) is faulty/corrupt, all the datagrams that
> > > were parsed properly in the first NTB and saved in rx_list are dropped.
> > >
> > > Adding a few custom traces showed the following:
> > >
> > > [002] d..1  7828.532866: dwc3_gadget_giveback: ep1out:
> > > req 000000003868811a length 1025/16384 zsI ==> 0
> > > [002] d..1  7828.532867: ncm_unwrap_ntb: K: ncm_unwrap_ntb toprocess: 1025
> > > [002] d..1  7828.532867: ncm_unwrap_ntb: K: ncm_unwrap_ntb nth: 1751999342
> > > [002] d..1  7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb seq: 0xce67
> > > [002] d..1  7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb blk_len: 0x400
> > > [002] d..1  7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb ndp_len: 0x10
> > > [002] d..1  7828.532869: ncm_unwrap_ntb: K: Parsed NTB with 1 frames
> > >
> > > In this case, the giveback is of 1025 bytes and block length is 1024.
> > > The rest 1 byte (which is 0x00) won't be parsed resulting in drop of
> > > all datagrams in rx_list.
> > >
> > > Same is case with packets of size 2048:
> > > [002] d..1  7828.557948: dwc3_gadget_giveback: ep1out:
> > > req 0000000011dfd96e length 2049/16384 zsI ==> 0
> > > [002] d..1  7828.557949: ncm_unwrap_ntb: K: ncm_unwrap_ntb nth: 1751999342
> > > [002] d..1  7828.557950: ncm_unwrap_ntb: K: ncm_unwrap_ntb blk_len: 0x800
> > >
> > > Lecroy shows one byte coming in extra confirming that the byte is coming
> > > in from PC:
> > >
> > > Transfer 2959 - Bytes Transferred(1025)  Timestamp((18.524 843 590)
> > > - Transaction 8391 - Data(1025 bytes) Timestamp(18.524 843 590)
> > > --- Packet 4063861
> > >       Data(1024 bytes)
> > >       Duration(2.117us) Idle(14.700ns) Timestamp(18.524 843 590)
> > > --- Packet 4063863
> > >       Data(1 byte)
> > >       Duration(66.160ns) Time(282.000ns) Timestamp(18.524 845 722)
> > >
> > > According to Windows driver, no ZLP is needed if wBlockLength is non-zero,
> > > because the non-zero wBlockLength has already told the function side the
> > > size of transfer to be expected. However, there are in-market NCM devices
> > > that rely on ZLP as long as the wBlockLength is multiple of wMaxPacketSize.
> > > To deal with such devices, it pads an extra 0 at end so the transfer is no
> > > longer multiple of wMaxPacketSize.
> > >
> > > Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
> > > ---
> > >  drivers/usb/gadget/function/f_ncm.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
> > > index ca5d5f564998..8c314dc98952 100644
> > > --- a/drivers/usb/gadget/function/f_ncm.c
> > > +++ b/drivers/usb/gadget/function/f_ncm.c
> > > @@ -1338,11 +1338,17 @@ static int ncm_unwrap_ntb(struct gether *port,
> > >              "Parsed NTB with %d frames\n", dgram_counter);
> > >
> > >         to_process -= block_len;
> > > -       if (to_process != 0) {
> > > +
> > > +       if (to_process == 1 &&
> > > +           (block_len % 512 == 0) &&
> > > +           (*(unsigned char *)(ntb_ptr + block_len) == 0x00)) {
> >
> > I'm unconvinced this (block_len % 512) works right...
> > imagine you have 2 ntbs back to back (for example 400 + 624) that
> > together add up to 1024,
> > and then a padding null byte.
> > I think block_len will be 624 then and not 1024.
> >
> > perhaps this actually needs to be:
> >   (ntp_ptr + block_len - skb->data) % 512 == 0
> >
> > The point is that AFAICT the multiple of 512/1024 that matters is in
> > the size of the USB transfer,
> > not the NTB block size itself.
> >
> > > +               goto done;
> > > +       } else if (to_process > 0) {
> > >                 ntb_ptr = (unsigned char *)(ntb_ptr + block_len);
> >
> > note that ntb_ptr moves forward by block_len here,
> > so it's *not* always the start of the packet, so block_len is not
> > necessarily the actual on the wire size.
> >
> > >                 goto parse_ntb;
> > >         }
> > >
> > > +done:
> > >         dev_consume_skb_any(skb);
> > >
> > >         return 0;
> > > --
> > > 2.34.1
> > >
> >
> > But it would perhaps be worth confirming in the MS driver source what
> > exactly they're doing...
> > (maybe they never even generate multiple ntbs per usb segment...)
> >
> > You may also want to mention in the commit message that 512 comes from
> > USB2 block size, and also works for USB3 block size of 1024.
>
> Also since this is a fix, it should probably have a Fixes tag
> (possibly on some original sha1 that added the driver, since I think
> it's always been broken) or at least a commit title that more clearly
> flags it as a 'fix' or cc stable...
>
> (something like 'usb: gadget: ncm: fix rare win11 packet discard')
>
> We definitely want this to hit LTS...

One more thing: the 'goto done' and 'done' label are not needed -
that's already the natural code flow...
So the 'goto' could just be replaced with a comment or perhaps with
--to_process.