lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c5f5e028-26b1-5c2d-ed7f-e36550ce6ac2@gmail.com>
Date:   Thu, 7 Apr 2022 22:51:52 +0200
From:   Lech Perczak <lech.perczak@...il.com>
To:     Bjørn Mork <bjorn@...k.no>
Cc:     netdev@...r.kernel.org, linux-usb@...r.kernel.org,
        Kristian Evensen <kristian.evensen@...il.com>,
        Oliver Neukum <oliver@...kum.org>
Subject: Re: [PATCH 2/3] rndis_host: enable the bogus MAC fixup for ZTE
 devices from cdc_ether

Hi Bjørn,

Many thanks you for your review! Answers inline.

W dniu 2022-04-07 o 08:25, Bjørn Mork pisze:
> Lech Perczak <lech.perczak@...il.com> writes:
>
>> +static int zte_rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>> +{
>> +	return rndis_rx_fixup(dev, skb) && usbnet_cdc_zte_rx_fixup(dev, skb);
>> +}
>>   
> Does this work as expected? Only the last ethernet packet in the rndis
> frame will end up being handled by usbnet_cdc_zte_rx_fixup().  The
> others are cloned and submitted directly to usbnet_skb_return().
I've got some positive reports from at least two owners of the device - 
I don't have one myself. In the meantime asked them to run tests with 
high traffic, because this should most probably manifest itself in that 
scenario easily - my wild guess is that the modem doesn't use batching, 
but you are most certainly right in the general case. And for testing on 
older modems, we can probably only count on Kristian.
>
> I don't know how to best solve that, but maybe add another
> RNDIS_DRIVER_DATA_x flag and test that in rndis_rx_fixup?  I.e something
> like
>
> 	bool fixup_dst = dev->driver_info->data & RNDIS_DRIVER_DATA_FIXUP_DST:
>          ..
>
> 		/* try to return all the packets in the batch */
> 		skb2 = skb_clone(skb, GFP_ATOMIC);
> 		if (unlikely(!skb2))
> 			break;
> 		skb_pull(skb, msg_len - sizeof *hdr);
> 		skb_trim(skb2, data_len);
>                  if (fixup_dst)
>                  	usbnet_cdc_zte_rx_fixup(dev, skb2);
> 		usbnet_skb_return(dev, skb2);
> 	}
>          if (fixup_dst)
>                  usbnet_cdc_zte_rx_fixup(dev, skb);
>
> 	/* caller will usbnet_skb_return the remaining packet */
> 	return 1;
> }

I'll consider that. My concern with that approach is degradation of 
performance by testing for that flag, both for ZTE and non-ZTE devices, 
for each and every packet. But this might be the only solution, as I 
cannot catch the n-1 sk_buffs from the batch mid-flight, only the last 
one. The only other way that currently comes to my mind, is to duplicate 
rndis_rx_fixup, with added calls to usbnet_cdc_zte_rx_fixup in the right 
places. But the amount of duplicated code by doing so would be huge, so 
I'd like to avoid that as well.

I will definitely send a V2 after I decide on a solution and do some 
testing, including high downlink traffic.

>
>
>
> Bjørn

-- 
Pozdrawiam/Kind regards,
Lech Perczak

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ