Received-SPF: pass (google.com: domain of linux-kernel+bounces-14265-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <ad60f399-5c6a-4f16-8c28-f4d4e0fde1ff@quicinc.com>
Date: Tue, 2 Jan 2024 16:18:47 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] usb: gadget: ncm: Avoid dropping datagrams of properly
 parsed NTBs
To: =?UTF-8?Q?Maciej_=C5=BBenczykowski?= <maze@google.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>
References: <20240102055143.3889-1-quic_kriskura@quicinc.com>
 <CANP3RGeirg+f8cBbw_3YR5AvuB1ZxJC_9-wcn+Tb-GXf1ESKCQ@mail.gmail.com>
Content-Language: en-US
From: Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
In-Reply-To: <CANP3RGeirg+f8cBbw_3YR5AvuB1ZxJC_9-wcn+Tb-GXf1ESKCQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


>> 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.
> 
> I think this is likely Windows trying to avoid generating 0 length frames.
> 
> (usb max single datagram [frame?] size is 1024 bytes).
> 
> My guess is this extra byte will only ever happen at the end of a
> multiple of 1024 bytes,
> and it will always be exactly one byte, and it will likely be a '0' pad byte.
> 

You are right. This happens only with packet sizes of 1024/2048/3072 
etc., and it is 0x00 only.

> Could you check if a more specific test of this sort would make sense?
> (ie. fix the problem)
> 
> Something like
> 
> if (to_process == 1) && (current_offset & 1023 == 0) && (*payload == 0)
>    // extra 1 zero byte pad to prevent multiple of 1024 sized packet
>    return
> }
> 

The above might work. But just wanted to check why this 1 byte would 
come actually ? Any reason for this ? ZLP must not give a 1 byte packet 
of 1 byte AFAIK.

> It seems a little dangerous to just blindly ignore arbitrary amounts
> of trailing garbage...
> 

Yes. I agree, which is why I put a note in comment section of patch 
stating that this doesn't cover all cases, just the ones found in the 
testing so far. But the code suggestion you provided might actually work 
out. So something like the following ?

if (to_process == 1) && (block_len%1024 == 0) && (*payload == 0)
     // extra 1 zero byte pad to prevent multiple of 1024 sized packet
     return
} else if (to_process > 1) {
     goto parse_ntb;
}

Just modified in current_offset with block_len and checked it with 
%1024. Let me know if it is fine and I will give the change to testing 
team. The issue is easily reproducible.

Regards,
Krishna,

>>
>> 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)
>>
>> Fix this by checking if the leftover bytes before parsing next NTB is of
>> size more than the expected header.
>>
>> Fixes: 427694cfaafa ("usb: gadget: ncm: Handle decoding of multiple NTB's in unwrap call")
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>> There could probably be cases where the first NTB is proper and the second
>> NTB's header is proper but the NDP is corrupt, and in those cases too, all
>> the datagrams are dropped. But I haven't seen such case practically.
>>
>>   drivers/usb/gadget/function/f_ncm.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
>> index cc0ed29a4adc..a75b6dc8b0cb 100644
>> --- a/drivers/usb/gadget/function/f_ncm.c
>> +++ b/drivers/usb/gadget/function/f_ncm.c
>> @@ -1325,7 +1325,7 @@ 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 > opts->nth_size) {
> 
> shouldn't this check actually be >= not > ?
> 

Yes. But what is header is present and no data further ? Just to fix 
another harmless corner case, I added the "=".

Regards,
Krishna,