Re: [RFC PATCH] usb: gadget: ncm: Fix handling of zero block length packets

[Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>]

On 2/27/2024 8:10 AM, Krishna Kurapati PSSNV wrote:
> 

>>
>> In general this is of course fine (though see Greg's auto-complaint).
>>
>> I haven't thought too much about this, but I just wonder whether the
>> check for block_len == 0
>> shouldn't be just after block_len is read, ie. somewhere just after:
>>
>> block_len = get_ncm(&tmp, opts->block_length);
>>
>> as it is kind of weird to be handling block_len == 0 at the point where
>> you are already theoretically done processing the block...
>>
>> I guess, as is, this assumes the block isn't actually of length 0,
>> since there's a bunch of following get_ncm() calls...
>> Are those guaranteed to be valid?
>>
> 
> I did get this doubt and tried it. I bailed out as soon as I found out 
> block len is zero without actually processing the datagrams present and 
> when I did that even ping doesn't work. Everything works only when the 
> datagrams in this zero block len NTB are parsed properly.
> 
>> I guess I don't actually see the infinite loop with block_len == 0,
>> since get_ncm() always moves us forward...
>>
> 
> The infinite loop occurs because we keep moving the buffer pointer 
> forward and keep processing the giveback until to_process variable 
> becomes zero or one. In case block length is zero, we never move the 
> buffer pointer forward and never reduce to_process variable and hence 
> keep infinitely processing the same NTB over and over again.
> 
>> Maybe your patch *is* correct as is, and you just need a comment
>> explaining *why* block_len == 0 is terminal at the spot you're adding 
>> the check.
>>
>> Also couldn't you fix this without goto, by changing
>>
>>    } else if (to_process > 0) {
>> to
>>    } else if (to_process && block_len) {
>>      // See NCM spec.  zero block_len means short packet.
>>
> 
> I will test this out once (although I know that looking at it, it would 
> definitely work) and send v2 with this diff.
> 
> Thanks for the review.
> 

Hi Maciej, Greg,

  Thanks for approving v2.

  Not sure if this is the right forum to ask this question, but had one 
query. In the NCM driver, the register_netdev is called during bind but 
the cleanup for that is called during free_inst. Meaning if usb0 
interface is created for ncm on bind or a composition switch into NCM 
(first comp switch after bootup), then it is removed only after removing 
the entire g1/functions/ncm.0 folder.

  Shouldn't we cleanup and remove the usb0 interface in unbind as a 
counter operation of bind ? By extension this question also applies to 
f_eem/ f_ecm/ f_rndis where it was done in similar manner. So was 
wondering if anyone could help me with info on why it was designed that way.

Regards,
Krishna,