Received-SPF: pass (google.com: domain of linux-kernel+bounces-46885-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240131150332.1326523-1-quic_kriskura@quicinc.com> <CANP3RGeHXmEcDN=-h1uGBzu_Ur2UcmiEuFDXAEr0Z2ptXnHq=Q@mail.gmail.com>
In-Reply-To: <CANP3RGeHXmEcDN=-h1uGBzu_Ur2UcmiEuFDXAEr0Z2ptXnHq=Q@mail.gmail.com>
From: =?UTF-8?Q?Maciej_=C5=BBenczykowski?= <maze@google.com>
Date: Wed, 31 Jan 2024 09:14:21 -0800
Message-ID: <CANP3RGeQ0-2vg0OeOHCuUzgeRRH4JyPw511Eoyy=HA-M9YAX2A@mail.gmail.com>
Subject: Re: [PATCH v2] usb: gadget: ncm: Avoid dropping datagrams of properly
 parsed NTBs
To: Krishna Kurapati <quic_kriskura@quicinc.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Hardik Gajjar <hgajjar@de.adit-jv.com>, 
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, 
	quic_ppratap@quicinc.com, quic_wcheng@quicinc.com, quic_jackp@quicinc.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 31, 2024 at 9:03=E2=80=AFAM Maciej =C5=BBenczykowski <maze@goog=
le.com> wrote:
>
> On Wed, Jan 31, 2024 at 7:03=E2=80=AFAM Krishna Kurapati
> <quic_kriskura@quicinc.com> wrote:
> >
> > It is observed sometimes when tethering is used over NCM with Windows 1=
1
> > as host, at some instances, the gadget_giveback has one byte appended a=
t
> > 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 pend=
ing
> > 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 th=
at
> > 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 =3D=3D> 0
> > [002] d..1  7828.532867: ncm_unwrap_ntb: K: ncm_unwrap_ntb toprocess: 1=
025
> > [002] d..1  7828.532867: ncm_unwrap_ntb: K: ncm_unwrap_ntb nth: 1751999=
342
> > [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: 0x4=
00
> > [002] d..1  7828.532868: ncm_unwrap_ntb: K: ncm_unwrap_ntb ndp_len: 0x1=
0
> > [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 =3D=3D> 0
> > [002] d..1  7828.557949: ncm_unwrap_ntb: K: ncm_unwrap_ntb nth: 1751999=
342
> > [002] d..1  7828.557950: ncm_unwrap_ntb: K: ncm_unwrap_ntb blk_len: 0x8=
00
> >
> > Lecroy shows one byte coming in extra confirming that the byte is comin=
g
> > 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-ze=
ro,
> > because the non-zero wBlockLength has already told the function side th=
e
> > size of transfer to be expected. However, there are in-market NCM devic=
es
> > that rely on ZLP as long as the wBlockLength is multiple of wMaxPacketS=
ize.
> > 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@quicinc.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/f=
unction/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 -=3D block_len;
> > -       if (to_process !=3D 0) {
> > +
> > +       if (to_process =3D=3D 1 &&
> > +           (block_len % 512 =3D=3D 0) &&
> > +           (*(unsigned char *)(ntb_ptr + block_len) =3D=3D 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 =3D=3D 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 =3D (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...