| Message ID | 1335557509-32726-3-git-send-email-nhorman@tuxdriver.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 2012-04-27 at 16:11 -0400, Neil Horman wrote: > Eric Dumazet pointed out to me that the drop_monitor protocol has some holes in > its smp protections. Specifically, its possible to replace data->skb while its > being written. This patch corrects that by making data->skb and rcu protected > variable. That will prevent it from being overwritten while a tracepoint is > modifying it. > > Signed-off-by: Neil Horman <nhorman@tuxdriver.com> > Reported-by: Eric Dumazet <eric.dumazet@gmail.com> > CC: David Miller <davem@davemloft.net> > --- > net/core/drop_monitor.c | 70 ++++++++++++++++++++++++++++++++++++----------- > 1 files changed, 54 insertions(+), 16 deletions(-) Acked-by: Eric Dumazet <edumazet@google.com> Thanks Neil -- 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
On Fri, 2012-04-27 at 16:11 -0400, Neil Horman wrote: > Eric Dumazet pointed out to me that the drop_monitor protocol has some holes in > its smp protections. Specifically, its possible to replace data->skb while its > being written. This patch corrects that by making data->skb and rcu protected > variable. That will prevent it from being overwritten while a tracepoint is > modifying it. > > static void send_dm_alert(struct work_struct *unused) > { > struct sk_buff *skb; > - struct per_cpu_dm_data *data = &__get_cpu_var(dm_cpu_data); > + struct per_cpu_dm_data *data = &get_cpu_var(dm_cpu_data); > > /* > * Grab the skb we're about to send > */ > - skb = data->skb; > + skb = rcu_dereference_protected(data->skb, 1); > > /* > * Replace it with a new one > @@ -111,8 +134,10 @@ static void send_dm_alert(struct work_struct *unused) > /* > * Ship it! > */ > - genlmsg_multicast(skb, 0, NET_DM_GRP_ALERT, GFP_KERNEL); > + if (skb) > + genlmsg_multicast(skb, 0, NET_DM_GRP_ALERT, GFP_KERNEL); > > + put_cpu_var(dm_cpu_data); > } > Oh well, drop_monitor can still trigger alerts : Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161774] BUG: sleeping function called from invalid context at mm/slub.c:943 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161779] in_atomic(): 1, irqs_disabled(): 0, pid: 2103, name: kworker/0:2 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161782] Pid: 2103, comm: kworker/0:2 Not tainted 3.5.0-rc1+ #55 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161784] Call Trace: Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161793] [<ffffffff810697ca>] __might_sleep+0xca/0xf0 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161798] [<ffffffff811345a3>] kmem_cache_alloc_node+0x1b3/0x1c0 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161804] [<ffffffff8105578c>] ? queue_delayed_work_on+0x11c/0x130 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161808] [<ffffffff815343fb>] __alloc_skb+0x4b/0x230 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161813] [<ffffffffa00b0360>] ? reset_per_cpu_data+0x160/0x160 [drop_monitor] Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161817] [<ffffffffa00b022f>] reset_per_cpu_data+0x2f/0x160 [drop_monitor] Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161820] [<ffffffffa00b03ab>] send_dm_alert+0x4b/0xb0 [drop_monitor] Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161824] [<ffffffff810568e0>] process_one_work+0x130/0x4c0 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161827] [<ffffffff81058249>] worker_thread+0x159/0x360 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161830] [<ffffffff810580f0>] ? manage_workers.isra.27+0x240/0x240 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161834] [<ffffffff8105d403>] kthread+0x93/0xa0 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161839] [<ffffffff816be6d4>] kernel_thread_helper+0x4/0x10 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161843] [<ffffffff8105d370>] ? kthread_freezable_should_stop+0x80/0x80 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161846] [<ffffffff816be6d0>] ? gs_change+0xb/0xb Also synchronize_rcu() cant be called in reset_per_cpu_data() for the same reason. Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161865] BUG: scheduling while atomic: kworker/0:2/2103/0x00000002 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161881] Modules linked in: drop_monitor ip6table_filter ip6_tables ebtable_nat ebtables fuse ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_state nf_conntrack ipt_REJECT iptable_filter bridge stp rt61pci crc_itu_t rt2x00pci rt2x00lib eeprom_93cx6 igb ixgbe mdio Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161884] Pid: 2103, comm: kworker/0:2 Not tainted 3.5.0-rc1+ #55 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161885] Call Trace: Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161890] [<ffffffff816ab9c3>] __schedule_bug+0x48/0x54 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161895] [<ffffffff816b42d3>] __schedule+0x793/0x7e0 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161898] [<ffffffff811314b2>] ? set_track+0x62/0x1a0 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161901] [<ffffffff816b43d9>] schedule+0x29/0x70 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161904] [<ffffffff816b2a15>] schedule_timeout+0x2c5/0x340 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161907] [<ffffffffa00b022f>] ? reset_per_cpu_data+0x2f/0x160 [drop_monitor] Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161910] [<ffffffff815343fb>] ? __alloc_skb+0x4b/0x230 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161914] [<ffffffff816b3a1a>] wait_for_common+0x13a/0x180 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161917] [<ffffffff8106f1f0>] ? try_to_wake_up+0x2e0/0x2e0 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161920] [<ffffffffa00b022f>] ? reset_per_cpu_data+0x2f/0x160 [drop_monitor] Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161925] [<ffffffff810be860>] ? call_rcu_bh+0x20/0x20 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161928] [<ffffffffa00b0360>] ? reset_per_cpu_data+0x160/0x160 [drop_monitor] Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161931] [<ffffffff816b3b3d>] wait_for_completion+0x1d/0x20 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161934] [<ffffffff8105a47d>] wait_rcu_gp+0x4d/0x60 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161937] [<ffffffff8105a490>] ? wait_rcu_gp+0x60/0x60 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161941] [<ffffffff812a0101>] ? uuid_le_gen+0x1/0x30 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161944] [<ffffffff810bf364>] synchronize_sched+0x44/0x50 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161948] [<ffffffffa00b02b5>] reset_per_cpu_data+0xb5/0x160 [drop_monitor] Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161951] [<ffffffffa00b03ab>] send_dm_alert+0x4b/0xb0 [drop_monitor] Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161955] [<ffffffff810568e0>] process_one_work+0x130/0x4c0 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161957] [<ffffffff81058249>] worker_thread+0x159/0x360 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161960] [<ffffffff810580f0>] ? manage_workers.isra.27+0x240/0x240 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161963] [<ffffffff8105d403>] kthread+0x93/0xa0 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161966] [<ffffffff816be6d4>] kernel_thread_helper+0x4/0x10 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161970] [<ffffffff8105d370>] ? kthread_freezable_should_stop+0x80/0x80 Jun 4 09:03:46 edumazet-laptop kernel: [ 2999.161973] [<ffffffff816be6d0>] ? gs_change+0xb/0xb -- 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
On Mon, Jun 04, 2012 at 09:45:10AM +0200, Eric Dumazet wrote: > On Fri, 2012-04-27 at 16:11 -0400, Neil Horman wrote: > > Eric Dumazet pointed out to me that the drop_monitor protocol has some holes in > > its smp protections. Specifically, its possible to replace data->skb while its > > being written. This patch corrects that by making data->skb and rcu protected > > variable. That will prevent it from being overwritten while a tracepoint is > > modifying it. > > > > > static void send_dm_alert(struct work_struct *unused) > > { > > struct sk_buff *skb; > > - struct per_cpu_dm_data *data = &__get_cpu_var(dm_cpu_data); > > + struct per_cpu_dm_data *data = &get_cpu_var(dm_cpu_data); > > > > /* > > * Grab the skb we're about to send > > */ > > - skb = data->skb; > > + skb = rcu_dereference_protected(data->skb, 1); > > > > /* > > * Replace it with a new one > > @@ -111,8 +134,10 @@ static void send_dm_alert(struct work_struct *unused) > > /* > > * Ship it! > > */ > > - genlmsg_multicast(skb, 0, NET_DM_GRP_ALERT, GFP_KERNEL); > > + if (skb) > > + genlmsg_multicast(skb, 0, NET_DM_GRP_ALERT, GFP_KERNEL); > > > > + put_cpu_var(dm_cpu_data); > > } > > > > Oh well, drop_monitor can still trigger alerts : > Grr, Not sure why I didn't see this before. I'll take care of it shortly. Neil -- 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/core/drop_monitor.c b/net/core/drop_monitor.c index a221a5b..4e04cf6 100644 --- a/net/core/drop_monitor.c +++ b/net/core/drop_monitor.c @@ -46,7 +46,7 @@ static DEFINE_MUTEX(trace_state_mutex); struct per_cpu_dm_data { struct work_struct dm_alert_work; - struct sk_buff *skb; + struct sk_buff __rcu *skb; atomic_t dm_hit_count; struct timer_list send_timer; }; @@ -73,35 +73,58 @@ static int dm_hit_limit = 64; static int dm_delay = 1; static unsigned long dm_hw_check_delta = 2*HZ; static LIST_HEAD(hw_stats_list); +static int initalized = 0; static void reset_per_cpu_data(struct per_cpu_dm_data *data) { size_t al; struct net_dm_alert_msg *msg; struct nlattr *nla; + struct sk_buff *skb; + struct sk_buff *oskb = rcu_dereference_protected(data->skb, 1); al = sizeof(struct net_dm_alert_msg); al += dm_hit_limit * sizeof(struct net_dm_drop_point); al += sizeof(struct nlattr); - data->skb = genlmsg_new(al, GFP_KERNEL); - genlmsg_put(data->skb, 0, 0, &net_drop_monitor_family, - 0, NET_DM_CMD_ALERT); - nla = nla_reserve(data->skb, NLA_UNSPEC, sizeof(struct net_dm_alert_msg)); - msg = nla_data(nla); - memset(msg, 0, al); - atomic_set(&data->dm_hit_count, dm_hit_limit); + skb = genlmsg_new(al, GFP_KERNEL); + + if (skb) { + genlmsg_put(skb, 0, 0, &net_drop_monitor_family, + 0, NET_DM_CMD_ALERT); + nla = nla_reserve(skb, NLA_UNSPEC, + sizeof(struct net_dm_alert_msg)); + msg = nla_data(nla); + memset(msg, 0, al); + } else if (initalized) + schedule_work_on(smp_processor_id(), &data->dm_alert_work); + + /* + * Don't need to lock this, since we are guaranteed to only + * run this on a single cpu at a time. + * Note also that we only update data->skb if the old and new skb + * pointers don't match. This ensures that we don't continually call + * synchornize_rcu if we repeatedly fail to alloc a new netlink message. + */ + if (skb != oskb) { + rcu_assign_pointer(data->skb, skb); + + synchronize_rcu(); + + atomic_set(&data->dm_hit_count, dm_hit_limit); + } + } static void send_dm_alert(struct work_struct *unused) { struct sk_buff *skb; - struct per_cpu_dm_data *data = &__get_cpu_var(dm_cpu_data); + struct per_cpu_dm_data *data = &get_cpu_var(dm_cpu_data); /* * Grab the skb we're about to send */ - skb = data->skb; + skb = rcu_dereference_protected(data->skb, 1); /* * Replace it with a new one @@ -111,8 +134,10 @@ static void send_dm_alert(struct work_struct *unused) /* * Ship it! */ - genlmsg_multicast(skb, 0, NET_DM_GRP_ALERT, GFP_KERNEL); + if (skb) + genlmsg_multicast(skb, 0, NET_DM_GRP_ALERT, GFP_KERNEL); + put_cpu_var(dm_cpu_data); } /* @@ -123,9 +148,11 @@ static void send_dm_alert(struct work_struct *unused) */ static void sched_send_work(unsigned long unused) { - struct per_cpu_dm_data *data = &__get_cpu_var(dm_cpu_data); + struct per_cpu_dm_data *data = &get_cpu_var(dm_cpu_data); + + schedule_work_on(smp_processor_id(), &data->dm_alert_work); - schedule_work(&data->dm_alert_work); + put_cpu_var(dm_cpu_data); } static void trace_drop_common(struct sk_buff *skb, void *location) @@ -134,9 +161,16 @@ static void trace_drop_common(struct sk_buff *skb, void *location) struct nlmsghdr *nlh; struct nlattr *nla; int i; - struct per_cpu_dm_data *data = &__get_cpu_var(dm_cpu_data); + struct sk_buff *dskb; + struct per_cpu_dm_data *data = &get_cpu_var(dm_cpu_data); + rcu_read_lock(); + dskb = rcu_dereference(data->skb); + + if (!dskb) + goto out; + if (!atomic_add_unless(&data->dm_hit_count, -1, 0)) { /* * we're already at zero, discard this hit @@ -144,7 +178,7 @@ static void trace_drop_common(struct sk_buff *skb, void *location) goto out; } - nlh = (struct nlmsghdr *)data->skb->data; + nlh = (struct nlmsghdr *)dskb->data; nla = genlmsg_data(nlmsg_data(nlh)); msg = nla_data(nla); for (i = 0; i < msg->entries; i++) { @@ -158,7 +192,7 @@ static void trace_drop_common(struct sk_buff *skb, void *location) /* * We need to create a new entry */ - __nla_reserve_nohdr(data->skb, sizeof(struct net_dm_drop_point)); + __nla_reserve_nohdr(dskb, sizeof(struct net_dm_drop_point)); nla->nla_len += NLA_ALIGN(sizeof(struct net_dm_drop_point)); memcpy(msg->points[msg->entries].pc, &location, sizeof(void *)); msg->points[msg->entries].count = 1; @@ -170,6 +204,8 @@ static void trace_drop_common(struct sk_buff *skb, void *location) } out: + rcu_read_unlock(); + put_cpu_var(dm_cpu_data); return; } @@ -375,6 +411,8 @@ static int __init init_net_drop_monitor(void) data->send_timer.function = sched_send_work; } + initalized = 1; + goto out; out_unreg:
Eric Dumazet pointed out to me that the drop_monitor protocol has some holes in its smp protections. Specifically, its possible to replace data->skb while its being written. This patch corrects that by making data->skb and rcu protected variable. That will prevent it from being overwritten while a tracepoint is modifying it. Signed-off-by: Neil Horman <nhorman@tuxdriver.com> Reported-by: Eric Dumazet <eric.dumazet@gmail.com> CC: David Miller <davem@davemloft.net> --- net/core/drop_monitor.c | 70 ++++++++++++++++++++++++++++++++++++----------- 1 files changed, 54 insertions(+), 16 deletions(-)