netdev - Re: [PATCH] rtnetlink: expose value from SET_NETDEV_DEVTYPE via IFLA

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] [day] [month] [year] [list]
Message-ID: <20180830102026.GG2181@nanopsycho>
Date:   Thu, 30 Aug 2018 12:20:26 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Marcel Holtmann <marcel@...tmann.org>
Cc:     netdev <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH] rtnetlink: expose value from SET_NETDEV_DEVTYPE via
 IFLA_DEVTYPE attribute

Thu, Aug 30, 2018 at 12:13:40PM CEST, marcel@...tmann.org wrote:
>Hi Jiri,
>
>>>>> The name value from SET_NETDEV_DEVTYPE only ended up in the uevent sysfs
>>>>> file as DEVTYPE= information. To avoid any kind of race conditions
>>>>> between netlink messages and reading from sysfs, it is useful to add the
>>>>> same string as new IFLA_DEVTYPE attribute included in the RTM_NEWLINK
>>>>> messages.
>>>>> 
>>>>> For network managing daemons that have to classify ARPHRD_ETHER network
>>>>> devices into different types (like Wireless LAN, Bluetooth etc.), this
>>>>> avoids the extra round trip to sysfs and parsing of the uevent file.
>>>>> 
>>>>> Signed-off-by: Marcel Holtmann <marcel@...tmann.org>
>>>>> ---
>>>>> include/uapi/linux/if_link.h |  2 ++
>>>>> net/core/rtnetlink.c         | 12 ++++++++++++
>>>>> 2 files changed, 14 insertions(+)
>>>>> 
>>>>> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>>>>> index 43391e2d1153..781294972bb4 100644
>>>>> --- a/include/uapi/linux/if_link.h
>>>>> +++ b/include/uapi/linux/if_link.h
>>>>> @@ -166,6 +166,8 @@ enum {
>>>>> 	IFLA_NEW_IFINDEX,
>>>>> 	IFLA_MIN_MTU,
>>>>> 	IFLA_MAX_MTU,
>>>>> +	IFLA_DEVTYPE,		/* Name value from SET_NETDEV_DEVTYPE */
>>>> 
>>>> This is not something netdev-related. dev->dev.type is struct device_type.
>>>> This is a generic "device" thing. Incorrect to expose over
>>>> netdev-specific API. Please use "device" API for this.
>>> 
>>> it is not just "device" related since this is a sub-classification of ARPHRD_ETHER type as a wrote above. Don't get hang up that this information is part of struct device. The dev->dev.type contains strings like "wlan", "bluetooth", "wimax", "gadget" etc. so that you can tell what kind of Ethernet card you have.
>> 
>> I understand. But using dev->dev.type for that purpose seems like an
>> abuse. If this is subtype of netdev, it should not be stored in parent
>> device. I believe that you need to re-introduce this as a part of struct
>> net_device - preferably an enum - and that you can expose over rtnl (and
>> net-sysfs).
>
>it is not stored in the parent device. It is stored in the device of the netdev.

Yeah. That is what I ment. "Device struct" associated with the
netdevice.

>
>As Stephen said, there is no device API. And why would I add the same info again and bloat netdev?
>
>drivers/net/bonding/bond_main.c:        SET_NETDEV_DEVTYPE(bond_dev, &bond_type);
>drivers/net/geneve.c:   SET_NETDEV_DEVTYPE(dev, &geneve_type);
>drivers/net/macsec.c:   SET_NETDEV_DEVTYPE(dev, &macsec_type);
>drivers/net/ppp/ppp_generic.c:  SET_NETDEV_DEVTYPE(dev, &ppp_type);
>drivers/net/usb/hso.c:  SET_NETDEV_DEVTYPE(net, &hso_type);
>drivers/net/usb/usbnet.c:               SET_NETDEV_DEVTYPE(net, &wlan_type);
>drivers/net/usb/usbnet.c:               SET_NETDEV_DEVTYPE(net, &wwan_type);
>drivers/net/vxlan.c:    SET_NETDEV_DEVTYPE(dev, &vxlan_type);
>drivers/net/wimax/i2400m/usb.c: SET_NETDEV_DEVTYPE(net_dev, &i2400mu_type);
>drivers/net/wireless/intersil/prism54/islpci_dev.c:     SET_NETDEV_DEVTYPE(ndev, &wlan_type);
>drivers/staging/gdm724x/gdm_lte.c:              SET_NETDEV_DEVTYPE(net, &wwan_type);
>drivers/usb/gadget/function/u_ether.c:  SET_NETDEV_DEVTYPE(net, &gadget_type);
>drivers/usb/gadget/function/u_ether.c:  SET_NETDEV_DEVTYPE(net, &gadget_type);
>net/8021q/vlan_dev.c:   SET_NETDEV_DEVTYPE(dev, &vlan_type);
>net/bluetooth/6lowpan.c:        SET_NETDEV_DEVTYPE(netdev, &bt_type);
>net/bluetooth/bnep/core.c:      SET_NETDEV_DEVTYPE(dev, &bnep_type);
>net/bridge/br_device.c: SET_NETDEV_DEVTYPE(dev, &br_type);
>net/dsa/slave.c:        SET_NETDEV_DEVTYPE(slave_dev, &dsa_type);
>net/hsr/hsr_device.c:   SET_NETDEV_DEVTYPE(dev, &hsr_type);
>net/l2tp/l2tp_eth.c:    SET_NETDEV_DEVTYPE(dev, &l2tpeth_type);
>net/wireless/core.c:            SET_NETDEV_DEVTYPE(dev, &wiphy_type);
>
>So for all these devices we have to add duplicate information now?

The fact the things are done now it a wrong way does not justify
extending UAPI in the same wrong way. So yes, change them all if you
need it. The "ethernet-subtype" should not an attribute of "struct
device" but rather "struct net_device".

>
>I actually don't like that idea. I can rename it to IFLA_FOO or any other proposal, but adding the same information twice is pointless.

So don't expose the "struct device" attribute over rtnetlink. Use
"struct device"-specific interface for that.

>
>>> We can revive the patches for adding this information as IFLA_INFO_KIND, but last time they where reverted since NetworkManager did all the wrong decisions based on that. That part is fixed now and we can put that back and declare the sub-type classification of Ethernet device in two places. Or we just take the information that we added a long time ago for exactly this sub-classification and provide them to userspace via RTNL.
>> 
>> IFLA_INFO_KIND serves for different purpose. It is for drivers that
>> register rtnl_link_ops. I don't think it would be wise to mix it with
>> this.
>
>We could register rtnl_link_ops for these Ethernet drivers, but then we are duplicating the information as well.

That is not the purpose of "rtnl_link_ops".


>
>Regards
>
>Marcel
>