netdev - Re: [PATCH 1/1] Fix qmap header retrieval in qmimux_rx

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:   Fri, 21 Dec 2018 13:54:24 +0100
From:   Daniele Palmas <dnlplm@...il.com>
To:     Bjørn Mork <bjorn@...k.no>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH 1/1] Fix qmap header retrieval in qmimux_rx_fixup

Il giorno ven 21 dic 2018 alle ore 13:33 Bjørn Mork <bjorn@...k.no> ha scritto:
>
> Daniele Palmas <dnlplm@...il.com> writes:
>
> > This patch fixes qmap header retrieval when modem is configured for
> > dl data aggregation.
> >
> > Signed-off-by: Daniele Palmas <dnlplm@...il.com>
> > ---
> > Hi Bjørn and all,
> >
> > I'm facing an issue when using qmi_wwan with modem configured with dl data aggregation and qmap multiplexing, e.g. something like
> >
> > root@...22:~# qmicli -d /dev/cdc-wdm0 --wda-set-data-format=link-layer-protocol=raw-ip,ul-protocol=qmap,dl-protocol=qmap,dl-max-datagrams=20
> > [/dev/cdc-wdm0] Successfully set data format
> >                         QoS flow header: no
> >                     Link layer protocol: 'raw-ip'
> >        Uplink data aggregation protocol: 'qmap'
> >      Downlink data aggregation protocol: 'qmap'
> >                           NDP signature: '0'
> > Downlink data aggregation max datagrams: '20'
> >      Downlink data aggregation max size: '16384'
> >
> > The issue is related to qmap header retrieval in qmimux_rx_fixup: basically it seems to me that it is always taken the first qmap header. Maybe the patch below should fix this issue.
> >
> > Note also that, by default, this won't be enough, since also rx_urb_size should be changed to the downlink data aggregation max size value: currently I'm just modifying the network interface MTU that changes also the
> > rx_urb_size.
> >
> > Not sure if this makes sense, so I thought to share anyway this with you for confirmation.
>
> My personal opinion - take it for that and nothing else:
>
> Aggregation adds buffer bloat, alignment issues, extra headers with
> associated magic handling, and more.  It can make sense for some use
> cases where each transmission adds significant overhead. The typical
> example is radio networks. But I do not think this applies to USB.  We
> are much better off sending each packet as a separate USB buffer, with a
> queue that is just long enough for back-to-back transmission.  The
> experience with aggregation in NCM/MBIM is not good.
>
> So I've ignored aggregation in QMI, and will probably continue doing
> so.  That doesn't mean that I am going to object to you or anyone else
> implementing the support if you see a usecase. That's your problem ;-)
>

Thanks for the feedback!

Regards,
Daniele

>
>
>
>
> Bjørn