Hi
This series implements a new sysfs attribute for netdevs called
"name_assign_type". It provides an integer that describes where an interface
name comes from. See Patch #1 for a description of this attribute. It is
modelled after the existing "addr_assign_type" attribute.
The main use-case is to allow udev to skip applying reliable ifnames to virtual
devices. For instance, if wifi-P2P devices are created, wpas already provides a
suitable naming-policy and udev shouldn't touch these devices. Same is true for
other virtual devices.
The idea is that if a device-name was provided by user-space, we should always
prefer fixing this naming-policy instead of making udev rename the device. For
kernel provided names that's hardly possible, though. Providing the
naming-policy source via sysfs is thus a simple way to see whether renames are
needed.
Additionally, this field allows to detect whether a netdev has been manually
renamed, which is quite useful for debugging and during crash-recovery.
Thanks
David
David Herrmann (4):
net: add name_assign_type netdev attribute
mac80211: set NET_NAME_USER for user-space created ifs
ath6kl: set NET_NAME_USER for P2P ifs
brcmfmac: set NET_NAME_USER for P2P ifs
drivers/net/wireless/ath/ath6kl/cfg80211.c | 5 ++++-
drivers/net/wireless/ath/ath6kl/cfg80211.h | 1 +
drivers/net/wireless/ath/ath6kl/core.c | 4 ++--
drivers/net/wireless/brcm80211/brcmfmac/p2p.c | 1 +
include/linux/netdevice.h | 6 ++++++
net/core/dev.c | 7 +++++++
net/core/net-sysfs.c | 2 ++
net/core/rtnetlink.c | 2 ++
net/mac80211/iface.c | 1 +
9 files changed, 26 insertions(+), 3 deletions(-)
--
1.9.0
The name_assign_type attribute gives hints where the interface name of a
given net-device comes from. Three different values are currently defined:
NET_NAME_ENUM:
This is the default. The ifname is provided by the kernel with an
enumerated suffix. Names may be reused and unstable.
NET_NAME_USER:
The ifname was provided by user-space during net-device setup.
NET_NAME_RENAMED:
The net-device has been renamed via RTNL. Once this type is set, it
cannot change again.
This attribute comes in handy for reliable net-device names. If an ifname
is provided by user-space, we can safely assume that the naming-policy
avoids reuse and is stable. Only if it was set by the kernel, the
interfaces might need to be renamed.
The NET_NAME_RENAMED value allows us to detect whether some-one else
already renamed the device, in which case we shouldn't touch it again. The
NET_NAME_USER value allows us to detect whether some other naming policy
created the device, in which case there's no need to rename it.
The most significant use-case is to detect virtual wifi-P2P devices, which
are named by wpa_supplicant et al. We shouldn't rename them as wpas
already provides a proper naming-policy.
Note that this patch only provides the core infrastructure. The different
net-dev types need to be manually fixed to use NET_NAME_USER instead of
the default (NET_NAME_ENUM). NET_NAME_ENUM is the least restrictive,
though, so it seems safe to use it as fallback for non-converted net-dev
types.
Signed-off-by: David Herrmann <[email protected]>
---
include/linux/netdevice.h | 6 ++++++
net/core/dev.c | 7 +++++++
net/core/net-sysfs.c | 2 ++
net/core/rtnetlink.c | 2 ++
4 files changed, 17 insertions(+)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index e8eeebd..d3a11c8 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -74,6 +74,11 @@ void netdev_set_default_ethtool_ops(struct net_device *dev,
#define NET_RX_SUCCESS 0 /* keep 'em coming, baby */
#define NET_RX_DROP 1 /* packet dropped */
+/* interface name assignment types */
+#define NET_NAME_ENUM 0 /* enumerated by kernel (default) */
+#define NET_NAME_USER 1 /* provided by user-space */
+#define NET_NAME_RENAMED 2 /* renamed by user-space */
+
/*
* Transmit return codes: transmit return codes originate from three different
* namespaces:
@@ -1165,6 +1170,7 @@ struct net_device {
* of the interface.
*/
char name[IFNAMSIZ];
+ unsigned char name_assign_type; /* name assignment type */
/* device name hash chain, please keep it close to name[] */
struct hlist_node name_hlist;
diff --git a/net/core/dev.c b/net/core/dev.c
index b1b0c8d..3608fb2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1083,6 +1083,7 @@ static int dev_get_valid_name(struct net *net,
int dev_change_name(struct net_device *dev, const char *newname)
{
char oldname[IFNAMSIZ];
+ unsigned char old_assign_type;
int err = 0;
int ret;
struct net *net;
@@ -1109,10 +1110,14 @@ int dev_change_name(struct net_device *dev, const char *newname)
return err;
}
+ old_assign_type = dev->name_assign_type;
+ dev->name_assign_type = NET_NAME_RENAMED;
+
rollback:
ret = device_rename(&dev->dev, dev->name);
if (ret) {
memcpy(dev->name, oldname, IFNAMSIZ);
+ dev->name_assign_type = old_assign_type;
write_seqcount_end(&devnet_rename_seq);
return ret;
}
@@ -1141,6 +1146,8 @@ rollback:
write_seqcount_begin(&devnet_rename_seq);
memcpy(dev->name, oldname, IFNAMSIZ);
memcpy(oldname, newname, IFNAMSIZ);
+ dev->name_assign_type = old_assign_type;
+ old_assign_type = NET_NAME_RENAMED;
goto rollback;
} else {
pr_err("%s: name change rollback failed: %d\n",
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 9388624..72a8ce5 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -104,6 +104,7 @@ static ssize_t netdev_store(struct device *dev, struct device_attribute *attr,
}
NETDEVICE_SHOW_RO(dev_id, fmt_hex);
+NETDEVICE_SHOW_RO(name_assign_type, fmt_dec);
NETDEVICE_SHOW_RO(addr_assign_type, fmt_dec);
NETDEVICE_SHOW_RO(addr_len, fmt_dec);
NETDEVICE_SHOW_RO(iflink, fmt_dec);
@@ -375,6 +376,7 @@ static struct attribute *net_class_attrs[] = {
&dev_attr_dev_id.attr,
&dev_attr_iflink.attr,
&dev_attr_ifindex.attr,
+ &dev_attr_name_assign_type.attr,
&dev_attr_addr_assign_type.attr,
&dev_attr_addr_len.attr,
&dev_attr_link_mode.attr,
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 1a0dac2..e10baa0 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1962,6 +1962,8 @@ replay:
}
dev->ifindex = ifm->ifi_index;
+ if (tb[IFLA_IFNAME])
+ dev->name_assign_type = NET_NAME_USER;
if (ops->newlink) {
err = ops->newlink(net, dev, tb, data);
--
1.9.0
On Thu, Feb 27, 2014 at 3:47 PM, David Herrmann <[email protected]> wrote:
> This series implements a new sysfs attribute for netdevs called
> "name_assign_type". It provides an integer that describes where an interface
> name comes from. See Patch #1 for a description of this attribute. It is
> modelled after the existing "addr_assign_type" attribute.
>
> The main use-case is to allow udev to skip applying reliable ifnames to virtual
> devices. For instance, if wifi-P2P devices are created, wpas already provides a
> suitable naming-policy and udev shouldn't touch these devices. Same is true for
> other virtual devices.
> The idea is that if a device-name was provided by user-space, we should always
> prefer fixing this naming-policy instead of making udev rename the device. For
> kernel provided names that's hardly possible, though. Providing the
> naming-policy source via sysfs is thus a simple way to see whether renames are
> needed.
>
> Additionally, this field allows to detect whether a netdev has been manually
> renamed, which is quite useful for debugging and during crash-recovery.
Moreover, it would be useful for udev to reliably know if some other
userspace process already renamed a device, so we know not to touch
it. This can easily happen for instance if some renaming happens in
the initrd or from script called from udev rules.
Acked-by: Tom Gundersen <[email protected]>
> David Herrmann (4):
> net: add name_assign_type netdev attribute
> mac80211: set NET_NAME_USER for user-space created ifs
> ath6kl: set NET_NAME_USER for P2P ifs
> brcmfmac: set NET_NAME_USER for P2P ifs
>
> drivers/net/wireless/ath/ath6kl/cfg80211.c | 5 ++++-
> drivers/net/wireless/ath/ath6kl/cfg80211.h | 1 +
> drivers/net/wireless/ath/ath6kl/core.c | 4 ++--
> drivers/net/wireless/brcm80211/brcmfmac/p2p.c | 1 +
> include/linux/netdevice.h | 6 ++++++
> net/core/dev.c | 7 +++++++
> net/core/net-sysfs.c | 2 ++
> net/core/rtnetlink.c | 2 ++
> net/mac80211/iface.c | 1 +
> 9 files changed, 26 insertions(+), 3 deletions(-)
>
> --
> 1.9.0
>
Netdevs created via nl80211 (currently only P2P ifs) have names provided
by user-space. Therefore, set the naming-policy to NET_NAME_USER so it is
correctly shown in sysfs.
Cc: Arend van Spriel <[email protected]>
Signed-off-by: David Herrmann <[email protected]>
---
drivers/net/wireless/brcm80211/brcmfmac/p2p.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/wireless/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/brcm80211/brcmfmac/p2p.c
index fc4f98b..6af4d26 100644
--- a/drivers/net/wireless/brcm80211/brcmfmac/p2p.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/p2p.c
@@ -2310,6 +2310,7 @@ struct wireless_dev *brcmf_p2p_add_vif(struct wiphy *wiphy, const char *name,
goto fail;
}
+ ifp->ndev->name_assign_type = NET_NAME_USER;
strncpy(ifp->ndev->name, name, sizeof(ifp->ndev->name) - 1);
err = brcmf_net_attach(ifp, true);
if (err) {
--
1.9.0
On 02/27/2014 03:47 PM, David Herrmann wrote:
> Netdevs created via nl80211 (currently only P2P ifs) have names provided
> by user-space. Therefore, set the naming-policy to NET_NAME_USER so it is
> correctly shown in sysfs.
>
- Cc: Arend van Spriel <[email protected]>
+ Acked-by: Arend van Spriel <[email protected]>
> Signed-off-by: David Herrmann <[email protected]>
> ---
> drivers/net/wireless/brcm80211/brcmfmac/p2p.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/brcm80211/brcmfmac/p2p.c
> index fc4f98b..6af4d26 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/p2p.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/p2p.c
> @@ -2310,6 +2310,7 @@ struct wireless_dev *brcmf_p2p_add_vif(struct wiphy *wiphy, const char *name,
> goto fail;
> }
>
> + ifp->ndev->name_assign_type = NET_NAME_USER;
> strncpy(ifp->ndev->name, name, sizeof(ifp->ndev->name) - 1);
> err = brcmf_net_attach(ifp, true);
> if (err) {
>
P2P netdevs and other devices that are created via nl80211 from user-space
have a name provided by user-space. Therefore, set NET_NAME_USER so this
is correctly shown in sysfs.
Cc: Kalle Valo <[email protected]>
Signed-off-by: David Herrmann <[email protected]>
---
drivers/net/wireless/ath/ath6kl/cfg80211.c | 5 ++++-
drivers/net/wireless/ath/ath6kl/cfg80211.h | 1 +
drivers/net/wireless/ath/ath6kl/core.c | 4 ++--
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c b/drivers/net/wireless/ath/ath6kl/cfg80211.c
index fd4c89d..74598ed 100644
--- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
+++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
@@ -1516,7 +1516,8 @@ static struct wireless_dev *ath6kl_cfg80211_add_iface(struct wiphy *wiphy,
return ERR_PTR(-EINVAL);
}
- wdev = ath6kl_interface_add(ar, name, type, if_idx, nw_type);
+ wdev = ath6kl_interface_add(ar, name, NET_NAME_USER, type,
+ if_idx, nw_type);
if (!wdev)
return ERR_PTR(-ENOMEM);
@@ -3625,6 +3626,7 @@ void ath6kl_cfg80211_vif_cleanup(struct ath6kl_vif *vif)
}
struct wireless_dev *ath6kl_interface_add(struct ath6kl *ar, const char *name,
+ unsigned char name_assign_type,
enum nl80211_iftype type,
u8 fw_vif_idx, u8 nw_type)
{
@@ -3661,6 +3663,7 @@ struct wireless_dev *ath6kl_interface_add(struct ath6kl *ar, const char *name,
ndev->dev_addr[4] ^= 0x80;
}
+ ndev->name_assign_type = name_assign_type;
init_netdev(ndev);
ath6kl_init_control_info(vif);
diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.h b/drivers/net/wireless/ath/ath6kl/cfg80211.h
index b59becd..5aa57a7 100644
--- a/drivers/net/wireless/ath/ath6kl/cfg80211.h
+++ b/drivers/net/wireless/ath/ath6kl/cfg80211.h
@@ -25,6 +25,7 @@ enum ath6kl_cfg_suspend_mode {
};
struct wireless_dev *ath6kl_interface_add(struct ath6kl *ar, const char *name,
+ unsigned char name_assign_type,
enum nl80211_iftype type,
u8 fw_vif_idx, u8 nw_type);
void ath6kl_cfg80211_ch_switch_notify(struct ath6kl_vif *vif, int freq,
diff --git a/drivers/net/wireless/ath/ath6kl/core.c b/drivers/net/wireless/ath/ath6kl/core.c
index 4b46adb..3cc8145 100644
--- a/drivers/net/wireless/ath/ath6kl/core.c
+++ b/drivers/net/wireless/ath/ath6kl/core.c
@@ -195,8 +195,8 @@ int ath6kl_core_init(struct ath6kl *ar, enum ath6kl_htc_type htc_type)
rtnl_lock();
/* Add an initial station interface */
- wdev = ath6kl_interface_add(ar, "wlan%d", NL80211_IFTYPE_STATION, 0,
- INFRA_NETWORK);
+ wdev = ath6kl_interface_add(ar, "wlan%d", NET_NAME_ENUM,
+ NL80211_IFTYPE_STATION, 0, INFRA_NETWORK);
rtnl_unlock();
--
1.9.0
David Herrmann <[email protected]> writes:
> P2P netdevs and other devices that are created via nl80211 from user-space
> have a name provided by user-space. Therefore, set NET_NAME_USER so this
> is correctly shown in sysfs.
>
> Cc: Kalle Valo <[email protected]>
> Signed-off-by: David Herrmann <[email protected]>
Looks good to me.
Acked-by: Kalle Valo <[email protected]>
Can I take this to my ath.git tree?
--
Kalle Valo
The nl80211 interface allows creating new netdevs from user-space. The
name is *always* provided by user-space, so we should set NET_NAME_USER to
provide that information via sysfs.
This allows udev to not rename dynamically created wifi devices (like wifi
P2P devices).
Cc: Johannes Berg <[email protected]>
Signed-off-by: David Herrmann <[email protected]>
---
net/mac80211/iface.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index ce1c443..35561bd 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1620,6 +1620,7 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name,
+ IEEE80211_ENCRYPT_HEADROOM;
ndev->needed_tailroom = IEEE80211_ENCRYPT_TAILROOM;
+ ndev->name_assign_type = NET_NAME_USER;
ret = dev_alloc_name(ndev, ndev->name);
if (ret < 0) {
free_netdev(ndev);
--
1.9.0
Kalle Valo <[email protected]> writes:
> David Herrmann <[email protected]> writes:
>
>> P2P netdevs and other devices that are created via nl80211 from user-space
>> have a name provided by user-space. Therefore, set NET_NAME_USER so this
>> is correctly shown in sysfs.
>>
>> Cc: Kalle Valo <[email protected]>
>> Signed-off-by: David Herrmann <[email protected]>
>
> Looks good to me.
>
> Acked-by: Kalle Valo <[email protected]>
>
> Can I take this to my ath.git tree?
Nevermind, I saw the first patch now. Just CCing random patches like
this is confusing for the recipients, better to CC the full patchset.
--
Kalle Valo
Given the premise of your patches,
> +++ b/net/mac80211/iface.c
> @@ -1620,6 +1620,7 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name,
> + IEEE80211_ENCRYPT_HEADROOM;
> ndev->needed_tailroom = IEEE80211_ENCRYPT_TAILROOM;
>
> + ndev->name_assign_type = NET_NAME_USER;
this is wrong because we call this in main.c with a "wlan%d" argument
for the name, which gets expanded.
johannes
Hi
On Thu, Feb 27, 2014 at 5:18 PM, Johannes Berg
<[email protected]> wrote:
> On Thu, 2014-02-27 at 15:56 +0100, David Herrmann wrote:
>
>> >> +++ b/net/mac80211/iface.c
>> >> @@ -1620,6 +1620,7 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name,
>> >> + IEEE80211_ENCRYPT_HEADROOM;
>> >> ndev->needed_tailroom = IEEE80211_ENCRYPT_TAILROOM;
>> >>
>> >> + ndev->name_assign_type = NET_NAME_USER;
>> >
>> > this is wrong because we call this in main.c with a "wlan%d" argument
>> > for the name, which gets expanded.
>>
>> Whoops, yeah, same issue as with ath6kl where I fixed it with a
>> separate "name_assign_type" argument. I will do the same here for v2.
>
> I'm not sure that helps - the user could potentially pass "foobar%d" as
> the interface name and get a number assigned by the kernel. In that case
> you probably *don't* want to set it to "name assigned by user"?
>
> Maybe this whole thing can just be handled in __dev_alloc_name() since
> no kernel user should pass just a string without "%d" in it.
Nope, I still want to set NET_NAME_USER in that case. It's not about
the %d suffix, it's about who chooses the suffix. If wpas decides to
use p2p-wlanX-%d, this is fine, as it's a virtual device where stable
IDs are not required at all.
Thanks
David
Hi
On Thu, Feb 27, 2014 at 3:53 PM, Johannes Berg
<[email protected]> wrote:
> Given the premise of your patches,
>
>> +++ b/net/mac80211/iface.c
>> @@ -1620,6 +1620,7 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name,
>> + IEEE80211_ENCRYPT_HEADROOM;
>> ndev->needed_tailroom = IEEE80211_ENCRYPT_TAILROOM;
>>
>> + ndev->name_assign_type = NET_NAME_USER;
>
> this is wrong because we call this in main.c with a "wlan%d" argument
> for the name, which gets expanded.
Whoops, yeah, same issue as with ath6kl where I fixed it with a
separate "name_assign_type" argument. I will do the same here for v2.
Thanks
David
On Thu, 2014-02-27 at 15:56 +0100, David Herrmann wrote:
> >> +++ b/net/mac80211/iface.c
> >> @@ -1620,6 +1620,7 @@ int ieee80211_if_add(struct ieee80211_local *local, const char *name,
> >> + IEEE80211_ENCRYPT_HEADROOM;
> >> ndev->needed_tailroom = IEEE80211_ENCRYPT_TAILROOM;
> >>
> >> + ndev->name_assign_type = NET_NAME_USER;
> >
> > this is wrong because we call this in main.c with a "wlan%d" argument
> > for the name, which gets expanded.
>
> Whoops, yeah, same issue as with ath6kl where I fixed it with a
> separate "name_assign_type" argument. I will do the same here for v2.
I'm not sure that helps - the user could potentially pass "foobar%d" as
the interface name and get a number assigned by the kernel. In that case
you probably *don't* want to set it to "name assigned by user"?
Maybe this whole thing can just be handled in __dev_alloc_name() since
no kernel user should pass just a string without "%d" in it.
johannes
On Thu, Feb 27, 2014 at 4:47 PM, Tom Gundersen <[email protected]> wrote:
> On Thu, Feb 27, 2014 at 3:47 PM, David Herrmann <[email protected]> wrote:
>> This series implements a new sysfs attribute for netdevs called
>> "name_assign_type". It provides an integer that describes where an interface
>> name comes from. See Patch #1 for a description of this attribute. It is
>> modelled after the existing "addr_assign_type" attribute.
>>
>> The main use-case is to allow udev to skip applying reliable ifnames to virtual
>> devices. For instance, if wifi-P2P devices are created, wpas already provides a
>> suitable naming-policy and udev shouldn't touch these devices. Same is true for
>> other virtual devices.
>> The idea is that if a device-name was provided by user-space, we should always
>> prefer fixing this naming-policy instead of making udev rename the device. For
>> kernel provided names that's hardly possible, though. Providing the
>> naming-policy source via sysfs is thus a simple way to see whether renames are
>> needed.
>>
>> Additionally, this field allows to detect whether a netdev has been manually
>> renamed, which is quite useful for debugging and during crash-recovery.
>
> Moreover, it would be useful for udev to reliably know if some other
> userspace process already renamed a device, so we know not to touch
> it. This can easily happen for instance if some renaming happens in
> the initrd or from script called from udev rules.
Incidentally, just after writing this email I ran into precisely this
problem due to buggy udev rules in a third-party package. The result
was that the upstream NIC naming rules were unable to detect that a
given NIC had already been renamed and ended up renaming them a second
time (hence wrecking havoc).
The problem was easy enough to fix, but with these patches we would be
able to avoid the issue altogether, so I'm looking forward to fixing
up udev to using this interface.
> Acked-by: Tom Gundersen <[email protected]>