netdev - Re: [PATCH net] qmi_wwan: Clone the skb when in pass-through mode

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]

Date:   Tue, 15 Jun 2021 11:03:24 +0200
From:   Kristian Evensen <kristian.evensen@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Bjørn Mork <bjorn@...k.no>,
        Network Development <netdev@...r.kernel.org>,
        subashab@...eaurora.org
Subject: Re: [PATCH net] qmi_wwan: Clone the skb when in pass-through mode

Hi Jakub,

On Mon, Jun 14, 2021 at 10:05 PM Jakub Kicinski <kuba@...nel.org> wrote:
> Agreed on the cloning being a strange solution. Kristian, were you able
> to reproduce the problem on upstream kernels?

Yes, after Bjørn's comment I realized that cloning was not a good
solution. I should have taken a closer look at the usbnet code, so
sorry about that. The most recent kernel I have managed to work well
with my boards is 5.4.123, but I see that 5.10 is available as well
(OpenWrt). However, I have backported all changes made to rmnet and
qmi_wwan between 5.4 and net-next as of yesterday. I was hoping that a
backport of the changes to those two drivers would be enough, but
perhaps there is something I have missed. I will try to get 5.10 to
run and see if that helps.

However, I have spent some more time looking into the code today.
Bjørn is right that calling netif_rx() from qmi_wwan is strange, at
least when in passthrough mode. The rx_fixup function will return 1
(assuming netif_rx() is successful), which in turn will lead to
usbnet_skb_return() being called and netif_rx() being called a second
time for the same skb. I have to admit that I don't know what will
happen when netif_rx() is called twice, but either call seems
redundant. I will submit a patch modifying the qmi_wwan rx_fixup
function to return 1 when the QMI_WWAN_FLAG_PASS_THROUGH. I believe it
is a nice clean-up and that is better to use as much of the existing
infrastructure as possible.

> It does look pretty strange that qmimux_rx_fixup() copies out all
> packets and receives them, and then let's usbnet to process the
> multi-frame skb without even fulling off the qmimux_hdr. I'm probably
> missing something.. otherwise sth like FLAG_MULTI_PACKET may be in
> order?

qmimux_rx_fixup() is different from what we are discussing here.
qmimux_rx_fixup() is used when the de-aggregation is performed by the
qmi_wwan driver, while the passthrough flag is set when the
de-aggregation is done by the rmnet driver. The logic in
qmimux_rx_fixup() is very similar to how the other usbnet mini-drivers
handles de-aggregation and also how de-aggregation is handled by for
example rmnet. I have no opinion on if the logic makes sens or not,
but at least the origin can be traced :)

Kristian