Message-ID: <CANP3RGeQ0-2vg0OeOHCuUzgeRRH4JyPw511Eoyy=HA-M9YAX2A@mail.gmail.com>
Date: Wed, 31 Jan 2024 09:14:21 -0800
From: Maciej Żenczykowski <maze@...gle.com>
To: Krishna Kurapati <quic_kriskura@...cinc.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Hardik Gajjar <hgajjar@...adit-jv.com>, 
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org, 
	quic_ppratap@...cinc.com, quic_wcheng@...cinc.com, quic_jackp@...cinc.com
Subject: Re: [PATCH v2] usb: gadget: ncm: Avoid dropping datagrams of properly
 parsed NTBs

On Wed, Jan 31, 2024 at 9:03 AM Maciej Żenczykowski <maze@...gle.com> wrote:
>
> On Wed, Jan 31, 2024 at 7:03 AM Krishna Kurapati
> <quic_kriskura@...cinc.com> 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@...cinc.com>
> > ---
> >  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...

Powered by blists - more mailing lists