Avoid multiple tree locks
Subsystems (13)
PATCH 1/1 Avoid multiple tree locks
Seems to have been around a while. IMO, mterial for 2.6.21 but not stable. I have only compile-tested but it looks right(tm). I could have moved the lock down, but this looked safer.
cheers,
jamal
[PKT_CLS] Avoid multiple tree locks
This fixes: When dumping filters the tree is locked first in the main dump function then when looking qdisc
---
commit 4a52cdd599f259b05320219d7aba1bac58fdf6d0
tree e9e4b83f7a2925b4408e4f18211365c3f9bff3fa
parent 0a14fe6e5efd0af0f9c6c01e0433445d615d0110
author Jamal Hadi Salim <[email protected]> Wed, 21 Mar 2007 05:27:55 -0400 committer Jamal Hadi Salim <[email protected]> Wed, 21 Mar 2007 05:27:55 -0400
include/net/pkt_sched.h | 1 + net/sched/cls_api.c | 2 +- net/sched/sch_api.c | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-)
include/net/pkt_sched.h +1 -0
@@ -212,6 +212,7 @@ extern struct Qdisc_ops bfifo_qdisc_ops;extern int register_qdisc (struct Qdisc_ops *qops);extern int unregister_qdisc (struct Qdisc_ops *qops); +extern struct Qdisc *__qdisc_lookup (struct net_device *dev,u32 handle);extern struct Qdisc *qdisc_lookup (struct net_device *dev,u32 handle);extern struct Qdisc *qdisc_lookup_class (struct net_device *dev,u32 handle); extern struct qdisc_rate_table *qdisc_get_rtab(struct tc_ratespec *r,
net/sched/cls_api.c +1 -1
@@ -403,7 +403,7 @@ static int tc_dump_tfilter(struct sk_buff *skb, struct netlink_callback *cb)if (!tcm->tcm_parent) q = dev->qdisc_sleeping;else - q =qdisc_lookup (dev,TC_H_MAJ (tcm->tcm_parent)); + q =__qdisc_lookup (dev,TC_H_MAJ (tcm->tcm_parent));if (!q) goto out;if ((cops = q->ops->cl_ops) ==NULL )
net/sched/sch_api.c +1 -1
@@ -190,7 +190,7 @@ int unregister_qdisc(struct Qdisc_ops *qops)(root qdisc, all its children, children of children etc.) */ -static struct Qdisc *__qdisc_lookup (struct net_device *dev,u32 handle) +struct Qdisc *__qdisc_lookup (struct net_device *dev,u32 handle) {struct Qdisc *q;
Quoted 2 lines · jamal
Seems to have been around a while. IMO, mterial for 2.6.21 but not stable. I have only compile-tested but it looks right(tm).
Its harmless since its a read lock, which can be nested. I actually don't see any need for qdisc_tree_lock at all, all changes and all walking is done under the RTNL, which is why I've removed it in my (upcoming) patches. I suggest to leave it as is for now so I don't need to change the __qdisc_lookup back to qdisc_lookup in 2.6.22.
Quoted 7 lines · Patrick McHardy
Quoted 2 lines · jamal
Seems to have been around a while. IMO, mterial for 2.6.21 but not stable. I have only compile-tested but it looks right(tm).
Quoted 6 lines
Its harmless since its a read lock, which can be nested. I actually don't see any need for
qdisc_tree_lockat all, all changes and all walking is done under the RTNL, which is why I've removed it in my (upcoming) patches. I suggest to leave it as is for now so I don't need to change the__qdisc_lookupback toqdisc_lookupin 2.6.22.
Alexey just explained to me why we do need qdisc_tree_lock in private mail. While dumping only the first skb is filled under the RTNL, while filling further skbs we don't hold the RTNL anymore. So I will probably have to drop that patch.
Quoted 9 lines · Patrick McHardy
Quoted 6 lines
Its harmless since its a read lock, which can be nested. I actually don't see any need for
qdisc_tree_lockat all, all changes and all walking is done under the RTNL, which is why I've removed it in my (upcoming) patches. I suggest to leave it as is for now so I don't need to change the__qdisc_lookupback toqdisc_lookupin 2.6.22.
Quoted 4 lines
Alexey just explained to me why we do need
qdisc_tree_lockin private mail. While dumping only the first skb is filled under the RTNL, while filling further skbs we don't hold the RTNL anymore. So I will probably have to drop that patch.
What we could do is replace the netlink cb_lock spinlock by a user-supplied mutex (supplied to netlink_kernel_create, rtnl_mutex in this case). That would put the entire dump under the rtnl and allow us to get rid of qdisc_tree_lock and avoid the need to take dev_base_lock during qdisc dumping. Same in other spots like rtnl_dump_ifinfo, inet_dump_ifaddr, ...
What do you think?
Quoted 7 lines · Patrick McHardy
Quoted 4 lines
Alexey just explained to me why we do need
qdisc_tree_lockin private mail. While dumping only the first skb is filled under the RTNL, while filling further skbs we don't hold the RTNL anymore. So I will probably have to drop that patch.
Quoted 6 lines
What we could do is replace the netlink
cb_lockspinlock by a user-supplied mutex (supplied tonetlink_kernel_create,rtnl_mutexin this case). That would put the entire dump under the rtnl and allow us to get rid ofqdisc_tree_lockand avoid the need to takedev_base_lockduring qdisc dumping. Same in other spots likertnl_dump_ifinfo,inet_dump_ifaddr, ...
These (compile tested) patches demonstrate the idea. The first one lets netlink_kernel_create users specify a mutex that should be held during dump callbacks, the second one uses this for rtnetlink and changes inet_dump_ifaddr for demonstration.
A complete patch would allow us to simplify locking in lots of spots, all rtnetlink users currently need to implement extra locking just for the dump functions, and a number of them already get it wrong and seem to rely on the rtnl.
If there are no objections to this change I'm going to update the second patch to include all rtnetlink users.
[NET_SCHED]: cls_basic: fix NULL pointer dereference
cls_basic doesn't allocate tp->root before it is linked into the active classifier list, resulting in a NULL pointer dereference when packets hit the classifier before its ->change function is called.
Reported by Chris Madden <[email protected]>
---
commit f1b9a0694552e18e7a43c292d21abe3b51dfcae2
tree f5ae39c1746fdc1ffbee6c1d90d035ee48ca4904
parent 0a14fe6e5efd0af0f9c6c01e0433445d615d0110
author Patrick McHardy <[email protected]> Tue, 20 Mar 2007 16:08:54 +0100 committer Patrick McHardy <[email protected]> Tue, 20 Mar 2007 16:08:54 +0100
net/sched/cls_basic.c | 16 +++++++--------- 1 files changed, 7 insertions(+), 9 deletions(-)
net/sched/cls_basic.c +7 -9
@@ -81,6 +81,13 @@ static void basic_put(struct tcf_proto *static int basic_init (struct tcf_proto *tp) { +struct basic_head *head; + + head =kzalloc (sizeof (*head),GFP_KERNEL ); +if (head ==NULL ) +return -ENOBUFS ; +INIT_LIST_HEAD (&head->flist); + tp->root = head;return 0 ; }
@@ -176,15 +183,6 @@ static int basic_change(struct tcf_proto } err = -ENOBUFS ; -if (head ==NULL ) { - head =kzalloc (sizeof (*head),GFP_KERNEL ); -if (head ==NULL ) - goto errout; - -INIT_LIST_HEAD (&head->flist); - tp->root = head; - } - f =kzalloc (sizeof (*f),GFP_KERNEL );if (f ==NULL ) goto errout;
[NET_SCHED]: Fix ingress locking
Ingress queueing uses a seperate lock for serializing enqueue operations, but fails to properly protect itself against concurrent changes to the qdisc tree. Use queue_lock for now since the real fix it quite intrusive.
---
commit 11985909b582dc688b5a7c0f73f16244224116f4
tree 0ee26bec34053f6c9b5f905ffbc1437881428eeb
parent f1b9a0694552e18e7a43c292d21abe3b51dfcae2
author Patrick McHardy <[email protected]> Tue, 20 Mar 2007 16:11:56 +0100 committer Patrick McHardy <[email protected]> Tue, 20 Mar 2007 16:11:56 +0100
net/core/dev.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
net/core/dev.c +2 -2
@@ -1750,10 +1750,10 @@ static int ing_filter(struct sk_buff *sk skb->tc_verd =SET_TC_AT (skb->tc_verd,AT_INGRESS ); -spin_lock (&dev->ingress_lock); +spin_lock (&dev->queue_lock);if ((q = dev->qdisc_ingress) !=NULL ) result = q->enqueue (skb, q); -spin_unlock (&dev->ingress_lock); +spin_unlock (&dev->queue_lock); }
Quoted 23 lines · Patrick McHardy
Quoted 7 lines · Patrick McHardy
Quoted 4 lines
Alexey just explained to me why we do need
qdisc_tree_lockin private mail. While dumping only the first skb is filled under the RTNL, while filling further skbs we don't hold the RTNL anymore. So I will probably have to drop that patch.Quoted 6 lines
What we could do is replace the netlink
cb_lockspinlock by a user-supplied mutex (supplied tonetlink_kernel_create,rtnl_mutexin this case). That would put the entire dump under the rtnl and allow us to get rid ofqdisc_tree_lockand avoid the need to takedev_base_lockduring qdisc dumping. Same in other spots likertnl_dump_ifinfo,inet_dump_ifaddr, ...These (compile tested) patches demonstrate the idea. The first one lets
netlink_kernel_createusers specify a mutex that should be held during dump callbacks, the second one uses this for rtnetlink and changesinet_dump_ifaddrfor demonstration.
Quoted 7 lines
A complete patch would allow us to simplify locking in lots of spots, all rtnetlink users currently need to implement extra locking just for the dump functions, and a number of them already get it wrong and seem to rely on the rtnl.
If there are no objections to this change I'm going to update the second patch to include all rtnetlink users
D'oh .. wrong patches.
[NETLINK]: Put dump callback under mutex, optionally user supplied
Replace the callback spinlock by a mutex and allow users to supply their own mutex to allow getting rid of seperate locking in dump callbacks. For users that don't supply their own mutex nothing changes.
---
commit c3400c45267a1fd291da75b0fe4b7970c846ff50
tree 96a4dc6050d74e72b4fffe9c047a0e695085e6db
parent 2c31e4429748f2629c59379b1113931a13a0cca9
author Patrick McHardy <[email protected]> Wed, 21 Mar 2007 14:43:02 +0100 committer Patrick McHardy <[email protected]> Wed, 21 Mar 2007 14:43:02 +0100
drivers/connector/connector.c | 2 +- drivers/scsi/scsi_netlink.c | 3 ++- drivers/scsi/scsi_transport_iscsi.c | 2 +- fs/ecryptfs/netlink.c | 2 +- include/linux/netlink.h | 5 ++++- lib/kobject_uevent.c | 2 +- net/bridge/netfilter/ebt_ulog.c | 2 +- net/core/rtnetlink.c | 2 +- net/decnet/netfilter/dn_rtmsg.c | 2 +- net/ipv4/fib_frontend.c | 2 +- net/ipv4/inet_diag.c | 2 +- net/ipv4/netfilter/ip_queue.c | 2 +- net/ipv4/netfilter/ipt_ULOG.c | 2 +- net/ipv6/netfilter/ip6_queue.c | 2 +- net/netfilter/nfnetlink.c | 2 +- net/netlink/af_netlink.c | 30 +++++++++++++++++++----------- net/netlink/genetlink.c | 2 +- net/xfrm/xfrm_user.c | 2 +- security/selinux/netlink.c | 2 +- 19 files changed, 41 insertions(+), 29 deletions(-)
drivers/connector/connector.c +1 -1
@@ -448,7 +448,7 @@ static int __devinit cn_init(void) dev->nls =netlink_kernel_create (NETLINK_CONNECTOR ,CN_NETLINK_USERS +0xf , - dev->input,THIS_MODULE ); + dev->input,NULL ,THIS_MODULE );if (!dev->nls)return -EIO ;
drivers/scsi/scsi_netlink.c +2 -1
@@ -168,7 +168,8 @@ scsi_netlink_init(void) } scsi_nl_sock =netlink_kernel_create (NETLINK_SCSITRANSPORT , -SCSI_NL_GRP_CNT , scsi_nl_rcv,THIS_MODULE ); +SCSI_NL_GRP_CNT , scsi_nl_rcv,NULL , +THIS_MODULE );if (!scsi_nl_sock) {printk (KERN_ERR "%s: register of recieve handler failed\n" , __FUNCTION__);
drivers/scsi/scsi_transport_iscsi.c +1 -1
@@ -1435,7 +1435,7 @@ static __init int iscsi_transport_init(void)if (err) goto unregister_conn_class; - nls =netlink_kernel_create (NETLINK_ISCSI ,1 , iscsi_if_rx, + nls =netlink_kernel_create (NETLINK_ISCSI ,1 , iscsi_if_rx,NULL ,THIS_MODULE );if (!nls) { err = -ENOBUFS ;
fs/ecryptfs/netlink.c +1 -1
@@ -229,7 +229,7 @@ int ecryptfs_init_netlink(void) ecryptfs_nl_sock =netlink_kernel_create (NETLINK_ECRYPTFS ,0 , ecryptfs_receive_nl_message, -THIS_MODULE ); +NULL ,THIS_MODULE );if (!ecryptfs_nl_sock) { rc = -EIO ;ecryptfs_printk (KERN_ERR ,"Failed to create netlink socket\n" );
include/linux/netlink.h +4 -1
@@ -157,7 +157,10 @@ struct netlink_skb_parms#define NETLINK_CREDS (skb) (&NETLINK_CB((skb)).creds) -extern struct sock *netlink_kernel_create (int unit,unsigned int groups,void (*input)(struct sock *sk,int len),struct module *module); +extern struct sock *netlink_kernel_create (int unit,unsigned int groups, +void (*input)(struct sock *sk,int len), +struct mutex *cb_mutex, +struct module *module);extern void netlink_ack (struct sk_buff *in_skb,struct nlmsghdr *nlh,int err);extern int netlink_has_listeners (struct sock *sk,unsigned int group);extern int netlink_unicast (struct sock *ssk,struct sk_buff *skb,__u32 pid,int nonblock);
lib/kobject_uevent.c +1 -1
@@ -293,7 +293,7 @@ EXPORT_SYMBOL_GPL(add_uevent_var);static int __init kobject_uevent_init (void ) { uevent_sock =netlink_kernel_create (NETLINK_KOBJECT_UEVENT ,1 ,NULL , -THIS_MODULE ); +NULL ,THIS_MODULE );if (!uevent_sock) { printk(KERN_ERR
net/bridge/netfilter/ebt_ulog.c +1 -1
@@ -304,7 +304,7 @@ static int __init ebt_ulog_init(void) } ebtulognl =netlink_kernel_create (NETLINK_NFLOG ,EBT_ULOG_MAXNLGROUPS , -NULL ,THIS_MODULE ); +NULL ,NULL ,THIS_MODULE );if (!ebtulognl) ret = -ENOMEM ;else if ((ret =ebt_register_watcher (&ulog)))
net/core/rtnetlink.c +1 -1
@@ -873,7 +873,7 @@ void __init rtnetlink_init(void)panic ("rtnetlink_init: cannot allocate rta_buf\n" ); rtnl =netlink_kernel_create (NETLINK_ROUTE ,RTNLGRP_MAX , rtnetlink_rcv, -THIS_MODULE ); +NULL ,THIS_MODULE );if (rtnl ==NULL )panic ("rtnetlink_init: cannot initialize rtnetlink\n" );netlink_set_nonroot (NETLINK_ROUTE ,NL_NONROOT_RECV );
net/decnet/netfilter/dn_rtmsg.c +1 -1
@@ -138,7 +138,7 @@ static int __init dn_rtmsg_init(void)int rv =0 ; dnrmg =netlink_kernel_create (NETLINK_DNRTMSG ,DNRNG_NLGRP_MAX , - dnrmg_receive_user_sk,THIS_MODULE ); + dnrmg_receive_user_sk,NULL ,THIS_MODULE );if (dnrmg ==NULL ) {printk (KERN_ERR "dn_rtmsg: Cannot create netlink socket" );return -ENOMEM ;
net/ipv4/fib_frontend.c +1 -1
@@ -816,7 +816,7 @@ static void nl_fib_input(struct sock *sk, int len)static void nl_fib_lookup_init (void ) { -netlink_kernel_create (NETLINK_FIB_LOOKUP ,0 , nl_fib_input,THIS_MODULE ); +netlink_kernel_create (NETLINK_FIB_LOOKUP ,0 , nl_fib_input,NULL ,THIS_MODULE ); }static void fib_disable_ip (struct net_device *dev,int force)
net/ipv4/inet_diag.c +1 -1
@@ -918,7 +918,7 @@ static int __init inet_diag_init(void) goto out; idiagnl =netlink_kernel_create (NETLINK_INET_DIAG ,0 , inet_diag_rcv, -THIS_MODULE ); +NULL ,THIS_MODULE );if (idiagnl ==NULL ) goto out_free_table; err =0 ;
net/ipv4/netfilter/ip_queue.c +1 -1
@@ -680,7 +680,7 @@ static int __init ip_queue_init(void)netlink_register_notifier (&ipq_nl_notifier); ipqnl =netlink_kernel_create (NETLINK_FIREWALL ,0 , ipq_rcv_sk, -THIS_MODULE ); +NULL ,THIS_MODULE );if (ipqnl ==NULL ) {printk (KERN_ERR "ip_queue: failed to create netlink socket\n" ); goto cleanup_netlink_notifier;
net/ipv4/netfilter/ipt_ULOG.c +1 -1
@@ -398,7 +398,7 @@ static int __init ipt_ulog_init(void) } nflognl =netlink_kernel_create (NETLINK_NFLOG ,ULOG_MAXNLGROUPS ,NULL , -THIS_MODULE ); +NULL ,THIS_MODULE );if (!nflognl)return -ENOMEM ;
net/ipv6/netfilter/ip6_queue.c +1 -1
@@ -669,7 +669,7 @@ static int __init ip6_queue_init(void)struct proc_dir_entry *proc;netlink_register_notifier (&ipq_nl_notifier); - ipqnl =netlink_kernel_create (NETLINK_IP6_FW ,0 , ipq_rcv_sk, + ipqnl =netlink_kernel_create (NETLINK_IP6_FW ,0 , ipq_rcv_sk,NULL ,THIS_MODULE );if (ipqnl ==NULL ) {printk (KERN_ERR "ip6_queue: failed to create netlink socket\n" );
net/netfilter/nfnetlink.c +1 -1
@@ -279,7 +279,7 @@ static int __init nfnetlink_init(void)printk ("Netfilter messages via NETLINK v%s.\n" , nfversion); nfnl =netlink_kernel_create (NETLINK_NETFILTER ,NFNLGRP_MAX , - nfnetlink_rcv,THIS_MODULE ); + nfnetlink_rcv,NULL ,THIS_MODULE );if (!nfnl) {printk (KERN_ERR "cannot initialize nfnetlink!\n" );return -1 ;
net/netlink/af_netlink.c +19 -11
@@ -76,7 +76,8 @@ struct netlink_sock {unsigned long state;wait_queue_head_t wait;struct netlink_callback *cb; -spinlock_t cb_lock; +struct mutex *cb_mutex; +struct mutex cb_def_mutex;void (*data_ready)(struct sock *sk,int bytes);struct module *module; };
@@ -108,6 +109,7 @@ struct netlink_table {unsigned long *listeners;unsigned int nl_nonroot;unsigned int groups; +struct mutex *cb_mutex;struct module *module;int registered; };
@@ -384,7 +386,7 @@ static int __netlink_create(struct socket *sock, int protocol)sock_init_data (sock, sk); nlk =nlk_sk (sk); -spin_lock_init (&nlk->cb_lock); +mutex_init (nlk->cb_mutex);init_waitqueue_head (&nlk->wait); sk->sk_destruct = netlink_sock_destruct;
@@ -395,6 +397,7 @@ static int __netlink_create(struct socket *sock, int protocol)static int netlink_create (struct socket *sock,int protocol) {struct module *module =NULL ; +struct mutex *cb_mutex;struct netlink_sock *nlk;unsigned int groups;int err =0 ;
@@ -419,6 +422,7 @@ static int netlink_create(struct socket *sock, int protocol) try_module_get(nl_table[protocol].module)) module = nl_table[protocol].module; groups = nl_table[protocol].groups; + cb_mutex = nl_table[protocol].cb_mutex;netlink_unlock_table ();if ((err =__netlink_create (sock, protocol)) <0 )
@@ -426,6 +430,7 @@ static int netlink_create(struct socket *sock, int protocol) nlk =nlk_sk (sock->sk); nlk->module = module; + nlk->cb_mutex = cb_mutex ? cb_mutex : &nlk->cb_def_mutex; out:return err;
@@ -445,14 +450,14 @@ static int netlink_release(struct socket *sock)netlink_remove (sk); nlk =nlk_sk (sk); -spin_lock (&nlk->cb_lock); +mutex_lock (nlk->cb_mutex);if (nlk->cb) {if (nlk->cb->done) nlk->cb->done (nlk->cb);netlink_destroy_callback (nlk->cb); nlk->cb =NULL ; } -spin_unlock (&nlk->cb_lock); +mutex_unlock (nlk->cb_mutex);/* OK. Socket is unlinked, and, therefore, no new packets will arrive */
@@ -1268,6 +1273,7 @@ static void netlink_data_ready(struct sock *sk, int len)struct sock *netlink_kernel_create (int unit,unsigned int groups,void (*input)(struct sock *sk,int len), +struct mutex *cb_mutex,struct module *module) {struct socket *sock;
@@ -1303,11 +1309,13 @@ netlink_kernel_create(int unit, unsigned int groups, nlk =nlk_sk (sk); nlk->flags |=NETLINK_KERNEL_SOCKET ; + nlk->cb_mutex = &nlk->cb_def_mutex;netlink_table_grab (); nl_table[unit].groups = groups; nl_table[unit].listeners = listeners; nl_table[unit].module = module; + nl_table[unit].cb_mutex = cb_mutex; nl_table[unit].registered =1 ;netlink_table_ungrab ();
@@ -1349,7 +1357,7 @@ static int netlink_dump(struct sock *sk)if (!skb) goto errout; -spin_lock (&nlk->cb_lock); +mutex_lock (nlk->cb_mutex); cb = nlk->cb;if (cb ==NULL ) {
@@ -1360,7 +1368,7 @@ static int netlink_dump(struct sock *sk) len = cb->dump (skb, cb);if (len >0 ) { -spin_unlock (&nlk->cb_lock); +mutex_unlock (nlk->cb_mutex);skb_queue_tail (&sk->sk_receive_queue, skb); sk->sk_data_ready (sk, len);return 0 ;
@@ -1378,13 +1386,13 @@ static int netlink_dump(struct sock *sk)if (cb->done) cb->done (cb); nlk->cb =NULL ; -spin_unlock (&nlk->cb_lock); +mutex_unlock (nlk->cb_mutex);netlink_destroy_callback (cb);return 0 ; errout_skb: -spin_unlock (&nlk->cb_lock); +mutex_unlock (nlk->cb_mutex);kfree_skb (skb); errout:return err;
@@ -1416,15 +1424,15 @@ int netlink_dump_start(struct sock *ssk, struct sk_buff *skb, } nlk =nlk_sk (sk);/* A dump is in progress... */ -spin_lock (&nlk->cb_lock); +mutex_lock (nlk->cb_mutex);if (nlk->cb) { -spin_unlock (&nlk->cb_lock); +mutex_unlock (nlk->cb_mutex);netlink_destroy_callback (cb);sock_put (sk);return -EBUSY ; } nlk->cb = cb; -spin_unlock (&nlk->cb_lock); +mutex_unlock (nlk->cb_mutex);netlink_dump (sk);sock_put (sk);
net/netlink/genetlink.c +1 -1
@@ -586,7 +586,7 @@ static int __init genl_init(void)netlink_set_nonroot (NETLINK_GENERIC ,NL_NONROOT_RECV ); genl_sock =netlink_kernel_create (NETLINK_GENERIC ,GENL_MAX_ID , - genl_rcv,THIS_MODULE ); + genl_rcv,NULL ,THIS_MODULE );if (genl_sock ==NULL )panic ("GENL: Cannot initialize generic netlink\n" );
net/xfrm/xfrm_user.c +1 -1
@@ -2467,7 +2467,7 @@ static int __init xfrm_user_init(void)printk (KERN_INFO "Initializing XFRM netlink socket\n" ); nlsk =netlink_kernel_create (NETLINK_XFRM ,XFRMNLGRP_MAX , - xfrm_netlink_rcv,THIS_MODULE ); + xfrm_netlink_rcv,NULL ,THIS_MODULE );if (nlsk ==NULL )return -ENOMEM ;rcu_assign_pointer (xfrm_nl, nlsk);
security/selinux/netlink.c +1 -1
@@ -104,7 +104,7 @@ void selnl_notify_policyload(u32 seqno)static int __init selnl_init (void ) { - selnl =netlink_kernel_create (NETLINK_SELINUX ,SELNLGRP_MAX ,NULL , + selnl =netlink_kernel_create (NETLINK_SELINUX ,SELNLGRP_MAX ,NULL ,NULL ,THIS_MODULE );if (selnl ==NULL )panic ("SELinux: Cannot create netlink socket." );
net/core/rtnetlink.c +1 -1
@@ -873,7 +873,7 @@ void __init rtnetlink_init(void)panic ("rtnetlink_init: cannot allocate rta_buf\n" ); rtnl =netlink_kernel_create (NETLINK_ROUTE ,RTNLGRP_MAX , rtnetlink_rcv, -NULL ,THIS_MODULE ); + &rtnl_mutex,THIS_MODULE );if (rtnl ==NULL )panic ("rtnetlink_init: cannot initialize rtnetlink\n" );netlink_set_nonroot (NETLINK_ROUTE ,NL_NONROOT_RECV );
net/ipv4/devinet.c +2 -10
@@ -1183,17 +1183,13 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)int s_ip_idx, s_idx = cb->args[0 ]; s_ip_idx = ip_idx = cb->args[1 ]; -read_lock (&dev_base_lock);for (dev = dev_base, idx =0 ; dev; dev = dev->next, idx++) {if (idx < s_idx)continue ;if (idx > s_idx) s_ip_idx =0 ; -rcu_read_lock (); -if ((in_dev =__in_dev_get_rcu (dev)) ==NULL ) { -rcu_read_unlock (); +if ((in_dev =__in_dev_get_rtnl (dev)) ==NULL )continue ; - }for (ifa = in_dev->ifa_list, ip_idx =0 ; ifa; ifa = ifa->ifa_next, ip_idx++) {
@@ -1201,16 +1197,12 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)continue ;if (inet_fill_ifaddr (skb, ifa,NETLINK_CB (cb->skb).pid, cb->nlh->nlmsg_seq, -RTM_NEWADDR ,NLM_F_MULTI ) <=0 ) { -rcu_read_unlock (); +RTM_NEWADDR ,NLM_F_MULTI ) <=0 ) goto done; - } } -rcu_read_unlock (); } done: -read_unlock (&dev_base_lock); cb->args[0 ] = idx; cb->args[1 ] = ip_idx;
Quoted 17 lines · Patrick McHardy
Quoted 6 lines · Patrick McHardy
What we could do is replace the netlink
cb_lockspinlock by a user-supplied mutex (supplied tonetlink_kernel_create,rtnl_mutexin this case). That would put the entire dump under the rtnl and allow us to get rid ofqdisc_tree_lockand avoid the need to takedev_base_lockduring qdisc dumping. Same in other spots likertnl_dump_ifinfo,inet_dump_ifaddr, ...These (compile tested) patches demonstrate the idea.
The first one
letsnetlink_kernel_createusers specify a mutex that should be held during dump callbacks, the second one uses this for rtnetlink and changesinet_dump_ifaddrfor demonstration.
Quoted 5 lines
A complete patch would allow us to simplify locking in lots of spots, all rtnetlink users currently need to implement extra locking just for the dump functions, and a number of them already get it wrong and seem to rely on the rtnl.
The mutex is certainly a cleaner approach;
and a lot of the RCU protection would go away. I like it.
Knowing you i sense theres something clever in there that i am missing. I dont see how you could get rid of the tree locking since we need to protect against the data path still, no? Or are you looking at that as a separate effort?
Quoted 2 lines
If there are no objections to this change I'm going to update the second patch to include all rtnetlink users.
No objections here.
cheers,
jamal
Quoted 16 lines · jamal
Quoted 7 lines · Patrick McHardy
These (compile tested) patches demonstrate the idea.
The first one
letsnetlink_kernel_createusers specify a mutex that should be held during dump callbacks, the second one uses this for rtnetlink and changesinet_dump_ifaddrfor demonstration.Quoted 5 lines
A complete patch would allow us to simplify locking in lots of spots, all rtnetlink users currently need to implement extra locking just for the dump functions, and a number of them already get it wrong and seem to rely on the rtnl.
Quoted 2 lines
The mutex is certainly a cleaner approach;
and a lot of the RCU protection would go away. I like it.
Not as much as I initially thought, but at least we would have consistent locking for the dump callbacks.
Quoted 4 lines
Knowing you i sense theres something clever in there that i am missing. I dont see how you could get rid of the tree locking since we need to protect against the data path still, no? Or are you looking at that as a separate effort?
We can remove qdisc_tree_lock since with this patch all changes and all tree walking happen under the RTNL. We still need to keep dev->queue_lock for the data path.
I'll update the patches to include all rtnetlink users and repost in a day or two.
Quoted 7 lines · Patrick McHardy
Quoted 1 line · jamal
On Wed, 2007-21-03 at 15:04 +0100, Patrick McHardy wrote:
We can remove
qdisc_tree_locksince with this patch all changes and all tree walking happen under the RTNL.
We still need to keepdev->queue_lockfor the data path.
ok, that would work. Should have been obvious to me.
cheers,
jamal
David Miller
maintainerFrom: Patrick McHardy <[email protected]>
Date: Thu, 22 Mar 2007 12:36:17 +0100
Quoted 12 lines
Quoted 2 lines · jamal
The mutex is certainly a cleaner approach;
and a lot of the RCU protection would go away. I like it.Not as much as I initially thought, but at least we would have consistent locking for the dump callbacks.
Quoted 4 lines
Knowing you i sense theres something clever in there that i am missing. I dont see how you could get rid of the tree locking since we need to protect against the data path still, no? Or are you looking at that as a separate effort?
Quoted 6 lines
We can remove
qdisc_tree_locksince with this patch all changes and all tree walking happen under the RTNL. We still need to keepdev->queue_lockfor the data path.I'll update the patches to include all rtnetlink users and repost in a day or two.
The existing weird "first SKB only" locking is unintuitive to me as well, so I'm all for these mutex patches once you respin them FWIW.
Quoted 6 lines · Patrick McHardy
Its harmless since its a read lock, which can be nested. I actually don't see any need for
qdisc_tree_lockat all, all changes and all walking is done under the RTNL, which is why I've removed it in my (upcoming) patches. I suggest to leave it as is for now so I don't need to change the__qdisc_lookupback toqdisc_lookupin 2.6.22.
Sounds good to me.
cheers,
jamal