netdev - Re: [PATCH net-next] net: qmi_wwan: Add 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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 27 Jun 2018 18:38:56 -0600
From:   Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
To:     Bjørn Mork <bjorn@...k.no>
Cc:     dnlplm@...il.com, dcbw@...hat.com, davem@...emloft.net,
        netdev@...r.kernel.org,
        Aleksander Morgado <aleksander@...ksander.es>
Subject: Re: [PATCH net-next] net: qmi_wwan: Add pass through mode

> The concepte looks fine to me, but I have a few comments to the
> implementation below.
> 
> First: I missed the last part of the discussion around automatic
> detection of passthrough mode.  Could you give us a short summary of 
> the
> alternatives you tried and why they were dropped?

Hi Bjørn

The other approach was to explicitly check for the rx_handler of the 
qmi_wwan
netdev and if it was attached by rmnet driver (rmnet_rx_handler).
If it was indeed attached, then the packet would be queued to stack. I 
had
dropped this option since it would mean that atleast the rmnet 
rx_handler would
have to be exported and exposed through a new header. This also brings a
tight coupling between qmi_wwan and rmnet driver.

> IIUC, userspace will be responsible for doing something like this to 
> set
> up a rmnet map interface:
> 
>  1) set the qmi_wwan netdev mode to raw-ip using sysfs
>  2) set the qmi_wwan netdev mode to pass-through using syfs
>  3) bind an rmnet netdev to the qmi_wwan netdev using netlink
>  4) configure the device for raw-ip using qmi
>  5) configure the device for map using qmi
> 
> I've just had a quick glance at this, but this function looks like an
> almost exact copy of the raw_ip_store().  Why not share that code
> instead of copying it?
> 
> And while you're at it:  There is nothing preventing us from turning on
> raw-ip here instead of failing if it is off, us there?  I.e. why not
> set QMI_WWAN_FLAG_RAWIP when QMI_WWAN_FLAG_PASS_THROUGH is being set,
> and clear QMI_WWAN_FLAG_PASS_THROUGH when QMI_WWAN_FLAG_RAWIP is being
> cleared?
> 
> You're missing the inverse relationship, aren't you?  There is nothing
> preventing the user from turning off raw-ip again after setting
> pass-through.
> 
> Do we really need all the notifier stuff here? You don't change the
> qmi_wwan netdev since that's already taken care of when setting
> QMI_WWAN_FLAG_RAWIP.
> 
> AFAICS, QMI_WWAN_FLAG_PASS_THROUGH can be changed without any locking,
> notifications or netdev state restrictions. All it does is change the
> behaviour on rx, and there is no reason that can't be applied from the
> next packet even on a running interface.
> 
> We could make pass_through_store just call raw_ip_store (or the part
> of it which matters, factored out into a separate function), if
> necessary. The rest of pass_through_store just sets or clears the flag.
> There is no need to do more than that, is there?
> 
> And as noted above, raw_ip_store must also clear
> QMI_WWAN_FLAG_PASS_THROUGH if clearing QMI_WWAN_FLAG_RAWIP.
> 
> There is no need testing for rawip here, since you enforce that in
> pass_through_store().

I can make these changes. With this, steps 1 and 2 are combined.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project