Received-SPF: pass (google.com: domain of linux-kernel+bounces-46949-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <47bf719c-a5c1-473b-9fa0-8cad84f0721c@quicinc.com>
Date: Wed, 31 Jan 2024 23:38:26 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] 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: <20240131150332.1326523-1-quic_kriskura@quicinc.com>
 <CANP3RGeHXmEcDN=-h1uGBzu_Ur2UcmiEuFDXAEr0Z2ptXnHq=Q@mail.gmail.com>
Content-Language: en-US
From: Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
In-Reply-To: <CANP3RGeHXmEcDN=-h1uGBzu_Ur2UcmiEuFDXAEr0Z2ptXnHq=Q@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


On 1/31/2024 10:33 PM, Maciej Żenczykowski wrote:
>>
>> 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)) {
> 

Hi Maciej,

> 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.
> 

Ahh !!  So you mean, since this comes because the host doesn't want to 
send packets of size wMaxPacketSize on the wire, we need to consider the 
length of data parsed so far to be checked against 512/1024 and not 
block length.

But either ways, the implementation in the patch also the same thing in 
a different way right ? Process all NTB's present iteratively and while 
doing so, check if there is one byte left. So if there are multiple 
NTB's mixed up to form a packet of size 1024, even then, both the logics 
would converge onto the point where they find that one byte is left.

>> +               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.
> 

Apologies, I didn't understand this comment. By moving the ntb_ptr by 
block length, we are pointing to the (last byte/ start of the next NTB) 
right as we are fixing in [1] ?

>>                  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...)
> 

Yes they do and we made a fix for that in [1].


> 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.
> 

Sure. Will update the commit message accordingly.

[1]: 
https://lore.kernel.org/all/20230927105858.12950-1-quic_kriskura@quicinc.com/

Regards,
Krishna,