| Message ID | 4AF3F905.4030608@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
Eric Dumazet wrote: > David > > A more elegant patch will be possible for 2.6.33, but for 2.6.32, > I think following patch is needed (Please note I did not test it) > > (More elegant : use RCU lookups ;) , I'll wait for net-next-2.6 > upgrade as well) > > Thanks > > [PATCH] can: should not use __dev_get_by_index() without locks > > bcm_proc_getifname() is called with RTNL and dev_base_lock > not held. It calls __dev_get_by_index() without locks, and > this is illegal (might crash) > > Close the race by holding dev_base_lock and copying dev->name > in the protected section. > > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> Thanks for pointing this out. This is a quite old code section which is not used very often at runtime - and usually the netdevice is not removed at that time ;-) Btw. this is no excuse for that missing locking, sorry. Thanks for the fix! Compiled and tested successfully. Signed-off-by: Oliver Hartkopp <oliver@hartkopp.net> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Oliver Hartkopp <socketcan@hartkopp.net> Date: Fri, 06 Nov 2009 12:04:16 +0100 > Eric Dumazet wrote: >> [PATCH] can: should not use __dev_get_by_index() without locks >> >> bcm_proc_getifname() is called with RTNL and dev_base_lock >> not held. It calls __dev_get_by_index() without locks, and >> this is illegal (might crash) >> >> Close the race by holding dev_base_lock and copying dev->name >> in the protected section. >> >> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> ... > Signed-off-by: Oliver Hartkopp <oliver@hartkopp.net> Applied, thanks everyone. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/can/bcm.c b/net/can/bcm.c index 597da4f..e8d58f3 100644 --- a/net/can/bcm.c +++ b/net/can/bcm.c @@ -132,23 +132,27 @@ static inline struct bcm_sock *bcm_sk(const struct sock *sk) /* * procfs functions */ -static char *bcm_proc_getifname(int ifindex) +static char *bcm_proc_getifname(char *result, int ifindex) { struct net_device *dev; if (!ifindex) return "any"; - /* no usage counting */ + read_lock(&dev_base_lock); dev = __dev_get_by_index(&init_net, ifindex); if (dev) - return dev->name; + strcpy(result, dev->name); + else + strcpy(result, "???"); + read_unlock(&dev_base_lock); - return "???"; + return result; } static int bcm_proc_show(struct seq_file *m, void *v) { + char ifname[IFNAMSIZ]; struct sock *sk = (struct sock *)m->private; struct bcm_sock *bo = bcm_sk(sk); struct bcm_op *op; @@ -157,7 +161,7 @@ static int bcm_proc_show(struct seq_file *m, void *v) seq_printf(m, " / sk %p", sk); seq_printf(m, " / bo %p", bo); seq_printf(m, " / dropped %lu", bo->dropped_usr_msgs); - seq_printf(m, " / bound %s", bcm_proc_getifname(bo->ifindex)); + seq_printf(m, " / bound %s", bcm_proc_getifname(ifname, bo->ifindex)); seq_printf(m, " <<<\n"); list_for_each_entry(op, &bo->rx_ops, list) { @@ -169,7 +173,7 @@ static int bcm_proc_show(struct seq_file *m, void *v) continue; seq_printf(m, "rx_op: %03X %-5s ", - op->can_id, bcm_proc_getifname(op->ifindex)); + op->can_id, bcm_proc_getifname(ifname, op->ifindex)); seq_printf(m, "[%d]%c ", op->nframes, (op->flags & RX_CHECK_DLC)?'d':' '); if (op->kt_ival1.tv64) @@ -194,7 +198,8 @@ static int bcm_proc_show(struct seq_file *m, void *v) list_for_each_entry(op, &bo->tx_ops, list) { seq_printf(m, "tx_op: %03X %s [%d] ", - op->can_id, bcm_proc_getifname(op->ifindex), + op->can_id, + bcm_proc_getifname(ifname, op->ifindex), op->nframes); if (op->kt_ival1.tv64)
David A more elegant patch will be possible for 2.6.33, but for 2.6.32, I think following patch is needed (Please note I did not test it) (More elegant : use RCU lookups ;) , I'll wait for net-next-2.6 upgrade as well) Thanks [PATCH] can: should not use __dev_get_by_index() without locks bcm_proc_getifname() is called with RTNL and dev_base_lock not held. It calls __dev_get_by_index() without locks, and this is illegal (might crash) Close the race by holding dev_base_lock and copying dev->name in the protected section. Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> --- net/can/bcm.c | 19 ++++++++++++------- 1 files changed, 12 insertions(+), 7 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html