net: core: decouple ifalias get/set from rtnl lock
Subsystems (2)
PATCH v2 1/1 net: core: decouple ifalias get/set from rtnl lock
Device alias can be set by either rtnetlink (rtnl is held) or sysfs.
rtnetlink hold the rtnl mutex, sysfs acquires it for this purpose. Add an extra mutex for it plus a seqcount to get a consistent snapshot of the alias buffer.
This allows the sysfs path to not take rtnl and would later allow to not hold it when dumping ifalias.
---
Changes since v1:
- add a comment why dev->ifalias is freed
via kfree, not kfree_rcu.
include/linux/netdevice.h | 3 +- net/core/dev.c | 70 +++++++++++++++++++++++++++++++++++++++-------- net/core/net-sysfs.c | 17 ++++++------ net/core/rtnetlink.c | 13 +++++++-- 4 files changed, 80 insertions(+), 23 deletions(-)
include/linux/netdevice.h +2 -1
@@ -1632,7 +1632,7 @@ enum netdev_priv_flags {struct net_device {char name[IFNAMSIZ ];struct hlist_node name_hlist; -char *ifalias; +char __rcu *ifalias;/* * I/O specific fields * FIXME: Merge these and struct ifmap into one
@@ -3275,6 +3275,7 @@ void __dev_notify_flags(struct net_device *, unsigned int old_flags, unsigned int gchanges);int dev_change_name (struct net_device *,const char *);int dev_set_alias (struct net_device *,const char *,size_t ); +int dev_get_alias (const struct net_device *,char *,size_t );int dev_change_net_namespace (struct net_device *,struct net *,const char *);int __dev_set_mtu (struct net_device *,int );int dev_set_mtu (struct net_device *,int );
net/core/dev.c +59 -11
100 lines not on this page · raw message
net/core/net-sysfs.c +8 -9
@@ -391,10 +391,7 @@ static ssize_t ifalias_store(struct device *dev, struct device_attribute *attr,if (len >0 && buf[len -1 ] =='\n' ) --count; -if (!rtnl_trylock ()) -return restart_syscall (); ret =dev_set_alias (netdev, buf, count); -rtnl_unlock ();return ret <0 ? ret : len; }
@@ -403,13 +400,12 @@ static ssize_t ifalias_show(struct device *dev, struct device_attribute *attr, char *buf) { const struct net_device *netdev = to_net_dev(dev); +char tmp[IFALIASZ ];ssize_t ret =0 ; -if (!rtnl_trylock ()) -return restart_syscall (); -if (netdev->ifalias) - ret =sprintf (buf,"%s\n" , netdev->ifalias); -rtnl_unlock (); + ret =dev_get_alias (netdev, tmp,sizeof (tmp)); +if (ret >0 ) + ret =sprintf (buf,"%s\n" , tmp);return ret; }static DEVICE_ATTR_RW (ifalias);
@@ -1488,7 +1484,10 @@ static void netdev_release(struct device *d)BUG_ON (dev->reg_state !=NETREG_RELEASED ); -kfree (dev->ifalias); +/* no need to wait for rcu grace period: +* device is dead and about to be freed. +*/ +kfree (rcu_access_pointer (dev->ifalias));netdev_freemem (dev); }
net/core/rtnetlink.c +11 -2
@@ -1366,6 +1366,16 @@ static int nla_put_iflink(struct sk_buff *skb, const struct net_device *dev)return nla_put_u32 (skb,IFLA_LINK , ifindex); } +static noinline_for_stack int nla_put_ifalias(struct sk_buff *skb, +struct net_device *dev) +{ +char buf[IFALIASZ ]; +int ret; + + ret =dev_get_alias (dev, buf,sizeof (buf)); +return ret >0 ?nla_put_string (skb,IFLA_IFALIAS , buf) :0 ; +} +static int rtnl_fill_link_netnsid (struct sk_buff *skb,const struct net_device *dev) {
@@ -1425,8 +1435,7 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,nla_put_u8 (skb,IFLA_CARRIER ,netif_carrier_ok (dev)) || (dev->qdisc &&nla_put_string (skb,IFLA_QDISC , dev->qdisc->ops->id)) || - (dev->ifalias && -nla_put_string (skb,IFLA_IFALIAS , dev->ifalias)) || +nla_put_ifalias (skb, dev) ||nla_put_u32 (skb,IFLA_CARRIER_CHANGES ,atomic_read (&dev->carrier_changes)) || nla_put_u8(skb, IFLA_PROTO_DOWN, dev->proto_down))
Signature · 1 line
2.13.6
Quoted 21 lines · Florian Westphal
Device alias can be set by either rtnetlink (rtnl is held) or sysfs.
rtnetlink hold the rtnl mutex, sysfs acquires it for this purpose. Add an extra mutex for it plus a seqcount to get a consistent snapshot of the alias buffer.
net/core/dev.c +14
-0+int dev_get_alias (const struct net_device *dev,char *alias,size_t len) +{ +unsigned int seq; +int ret; + +for (;;) { +const char *name; + + ret =0 ; +rcu_read_lock (); + name =rcu_dereference (dev->ifalias); + seq =raw_seqcount_begin (&ifalias_rename_seq); +if (name) + ret =snprintf (alias, len,"%s" , name);
net/core/dev.c int dev_change_name(struct net_device *dev, const char *newname) · line 1336
Quoted 10 lines
net/core/dev.c +10
-0+rcu_read_unlock (); + +if (!read_seqcount_retry (&ifalias_rename_seq, seq)) +break ; + +cond_resched (); + } + +return ret; +}
I believe this too complex and not needed.
Just use RCU : A writer is supposed to work on a private copy, and _then_ publish the new pointer, so that a reader can not see mangled string.
We either copy the 'old' name or the 'new' one.
A seqcount is not needed, and wont prevent you from reading the value right before a change anyway.
Quoted 8 lines · Eric Dumazet
Just use RCU : A writer is supposed to work on a private copy, and _then_ publish the new pointer, so that a reader can not see mangled string.
We either copy the 'old' name or the 'new' one.
A seqcount is not needed, and wont prevent you from reading the value right before a change anyway.
Would you rather use kfree_rcu or unconditional synchronize_net() before releasing old memory?
Quoted 5 lines · Florian Westphal
Quoted 4 lines · Eric Dumazet
Just use RCU : A writer is supposed to work on a private copy, and _then_ publish the new pointer, so that a reader can not see mangled string.
Quoted 7 lines
Quoted 4 lines
We either copy the 'old' name or the 'new' one.
A seqcount is not needed, and wont prevent you from reading the value right before a change anyway.
Would you rather use
kfree_rcuor unconditionalsynchronize_net()before releasing old memory?
kfree_rcu() please ;)
Adding 16 bytes for the rcu_head is acceptable I think.