Message-ID: <916462f9-0fff-41f2-a187-a3a4fa57f7e1@pen.gy>
Date: Sun, 26 Jan 2025 14:44:31 +0100
From: Foster Snowhill <forst@....gy>
To: Bjørn Mork <bjorn@...k.no>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Georgi Valkov <gvalkov@...il.com>,
 Simon Horman <horms@...nel.org>, Oliver Neukum <oneukum@...e.com>,
 netdev@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH net v5 0/7] usbnet: ipheth: prevent OoB reads of NDP16

Hello Bjørn,

Thank you very much for the feedback! In my opinion, it's never late.

On 2025-01-26 12:19, Bjørn Mork wrote:
> We did a lot of that when prepping it for cdc_mbim
> reuse. Some of it, like supporting multiple NDPs, is completely useless
> for NCM.  We still added code to cdc_ncm just to be able to share it
> with cdc_mbim. I think the generalized solutions still adds value to
> cdc_ncm by making the shared code targeted by more developers and users.
> 
> And the huawei_cdc_ncm driver demonstrates that cdc_ncm can be reused
> for non-compliant vendor-specific protocols, so that's not a real
> problem.

Very much appreciate the pointers to look at how `cdc_mbim` and
`huawei_cdc_ncm` share code with `cdc_ncm`, I'm definitely interested
and will have a look.

> Sorry for taking this up at such a late stage, and it might already been
> discussed, but I'm not convinced that you shouldn't "mess" with the
> `cdc_ncm` driver.
> 
> ...
> 
> What I can understand is that you don't want to build on the current
> cdc_ncm code because you don't like the implementation and want to
> improve it.  But this is also the main reason why I think code sharing
> would be good.  The cdc_ncm code could certainly do with more developers
> looking at it and improving stuff.  Like any other drivers, really.
> 
> Yes, I know I'm saying this much too late for code that's ready for
> merging.  And, of course, I don't expect you to start all over at this
> point. I just wanted to comment on that "messing with" because it's so
> wrong, and I remember feeling that way too.  Messing with existing code
> is never a problem in Linux!  Existing code and (internal) interfaces
> can always be changed to accommodate whatever needs you have.  This is
> one of may ways the Linux development model wins over everything else.

I call it "mess with it" not because I see something wrong with the
existing CDC drivers (I don't), but rather because of my own skill
level. This patch series is fixing mistakes I originally made when
implementing NCM mode in `ipheth`, so you can tell I still have a lot
of learning to do. Especially before touching better maintained and
afaik more widely used drivers like `cdc_ncm` and co.

I should've mentioned in the cover letter perhaps that I see this
series as more of an interim set of fixes to the issues I found
in my existing code. For longer term, I already have some ideas and
questions on further improving the `ipheth` driver, in particular:

1. Can I make `ipheth` use the `usbnet` framework, rather than it
   re-implementing lower-level things like USB interface handling?
2. How can I reuse parts of `cdc_ncm` to parse incoming URBs?
   Does it require complying with the `usbnet` framework?
3. How can I make use of multiple parallel USB transfers to improve
   throughput? Does `usbnet` support this already, or would it have
   to be implemented?
   Last time I checked, the official driver on macOS does 16 parallel
   transfers. This results in a significant useful throughput increase
   on RX on newer devices over 10 Gbps USB-C: from 650-850 Mbps on
   Linux with `ipheth` to ~3.1 Gbps on macOS.
4. How far back do I keep driver compatibility?
   So far all the changes have been non-breaking, I've been asking
   Georgi Valkov <gvalkov@...il.com> to test on devices as old as
   iPhone 3G with iOS 4.2.1, which pretty much covers all iOS devices
   and versions capable of internet sharing.
5. Even longer term, would be interesting to investigate the unknown
   control transfers that the official driver does. I avoid doing any
   reverse engineering myself ("clean room" principle), so thinking
   of maybe somehow emulating an iOS device and trying to return
   different values and observing how macOS behaves. Alternatively
   just asking someone else to do the reserve-engineering part, but
   not sure there's any interest in that.

To be clear, these questions are not for you specifically, Bjørn, just
my own notes on what I'd like to investigate next. But if you have any
insight or opinion on any the above, it's absolutely welcome.

These haven't been discussed before, so you didn't miss anything.
I didn't mention it in the cover letter because I'm not sure when I'll
be able to work on these items, didn't want to make promises.
Excuses-excuses, I know. :)

Powered by blists - more mailing lists