* Re: use-after-free in usbnet
From: Ming Lei @ 2012-04-21 2:02 UTC (permalink / raw)
To: Huajun Li
Cc: Oliver Neukum, Alan Stern, Dave Jones, netdev, linux-usb,
Fedora Kernel Team
In-Reply-To: <CACVXFVOc0XZ+eLHGiVwKuiUResRk8Cj9MS4EPMx7k57a0tEJhA@mail.gmail.com>
On Sat, Apr 21, 2012 at 9:49 AM, Ming Lei <tom.leiming@gmail.com> wrote:
> On Fri, Apr 20, 2012 at 10:56 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>> On Fri, Apr 20, 2012 at 10:22 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>>> On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>>>>
>>>> Above patch has already been integrated to mainline. However, maybe
>>>> there still exists another potentail use-after-free issue, here is a
>>>> case:
>>>> After release the lock in unlink_urbs(), defer_bh() may move
>>>> current skb from rxq/txq to dev->done queue, even cause the skb be
>>>> released. Then in next loop cycle, it can't refer to expected skb, and
>>>> may Oops again.
>>>
>>> Could you explain in a bit detail? Why can't the expected skb be refered
>>> to in next loop?
>>
>>
>> unlink_urbs() complete handler
>> --------------------------------------
>> -------------------------------------------------
>> spin_unlock_irqrestore()
>> rx_complete()
>> derver_bh()
>>
>> __skb_unlink()
>>
>> __skb_queue_tail(&dev->done, skb) =======> skb is moved to
>> dev->done, and can be freed by usbnet_bh()
>> skb_queue_walk_safe()
>> tmp = skb->next ===> refer to freed skb
>
> I see the problem, so looks skb_queue_walk_safe is not safe.
> I don' know why the 2nd ' tmp = skb->next' in skb_queue_walk_safe
> is needed and it may become unsafe if skb is freed during current loop.
>
> But removing the 2nd 'tmp = skb->next' doesn't help the problem, because
> tmp still may become freed after releasing lock.
>
>> If its state is x_done/tx_done/rx_cleanup, that means the the skb will
>> be released soon, right? If so, it should avoid calling
>> usb_unlink_urb().
>
> Even though you can avoid calling unlink for completed URBs, the skbs
> still may be freed in unlink path because complete handler will be triggered
> by unlink and the referenced skb may be freed before next loop, so your
> patch can't fix the oops.
>
> As far as I can think of, we can hold lock of done queue to forbid skb free
> during unlinking. The below patch may fix the problem, are you OK
> with it?
Sorry, the patch still doesn't work since done.lock can't be held
before calling unlinking.
One simple solution is just always holding q->lock during the whole loop,
like before commit 4231d47e6fe69f061f96c98c30eaf9fb4c14b96d
(net/usbnet: avoid recursive locking in usbnet_stop()), and take
the per cpu flag trick to avoid holding q->lock in complete handler
called by unlink, see idea in below link:
http://marc.info/?l=linux-usb&m=133491718707499&w=2
In theory, it should be simple, just take avoiding policy.
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index db99536..a9809d4 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -581,7 +581,8 @@ static int unlink_urbs (struct usbnet *dev, struct
> sk_buff_head *q)
> struct sk_buff *skb, *skbnext;
> int count = 0;
>
> - spin_lock_irqsave (&q->lock, flags);
> + spin_lock_irqsave(&dev->done.lock, flags);
> + spin_lock(&q->lock);
> skb_queue_walk_safe(q, skb, skbnext) {
> struct skb_data *entry;
> struct urb *urb;
> @@ -598,7 +599,7 @@ static int unlink_urbs (struct usbnet *dev, struct
> sk_buff_head *q)
> * handler(include defer_bh).
> */
> usb_get_urb(urb);
> - spin_unlock_irqrestore(&q->lock, flags);
> + spin_unlock(&q->lock);
> // during some PM-driven resume scenarios,
> // these (async) unlinks complete immediately
> retval = usb_unlink_urb (urb);
> @@ -607,9 +608,10 @@ static int unlink_urbs (struct usbnet *dev,
> struct sk_buff_head *q)
> else
> count++;
> usb_put_urb(urb);
> - spin_lock_irqsave(&q->lock, flags);
> + spin_lock(&q->lock);
> }
> - spin_unlock_irqrestore (&q->lock, flags);
> + spin_unlock(&q->lock);
> + spin_unlock_irqrestore(&dev->done.lock, flags);
> return count;
> }
>
>
>
> Thanks,
> --
> Ming Lei
--
Ming Lei
^ permalink raw reply
* Re: use-after-free in usbnet
From: Ming Lei @ 2012-04-21 1:49 UTC (permalink / raw)
To: Huajun Li
Cc: Oliver Neukum, Alan Stern, Dave Jones, netdev, linux-usb,
Fedora Kernel Team
In-Reply-To: <CA+v9cxZWtGbz6uCSysVbAc1hT27rCiuJXzcvSiTxH-zuQYnrZw@mail.gmail.com>
On Fri, Apr 20, 2012 at 10:56 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
> On Fri, Apr 20, 2012 at 10:22 PM, Ming Lei <tom.leiming@gmail.com> wrote:
>> On Fri, Apr 20, 2012 at 9:37 PM, Huajun Li <huajun.li.lee@gmail.com> wrote:
>>>
>>> Above patch has already been integrated to mainline. However, maybe
>>> there still exists another potentail use-after-free issue, here is a
>>> case:
>>> After release the lock in unlink_urbs(), defer_bh() may move
>>> current skb from rxq/txq to dev->done queue, even cause the skb be
>>> released. Then in next loop cycle, it can't refer to expected skb, and
>>> may Oops again.
>>
>> Could you explain in a bit detail? Why can't the expected skb be refered
>> to in next loop?
>
>
> unlink_urbs() complete handler
> --------------------------------------
> -------------------------------------------------
> spin_unlock_irqrestore()
> rx_complete()
> derver_bh()
>
> __skb_unlink()
>
> __skb_queue_tail(&dev->done, skb) =======> skb is moved to
> dev->done, and can be freed by usbnet_bh()
> skb_queue_walk_safe()
> tmp = skb->next ===> refer to freed skb
I see the problem, so looks skb_queue_walk_safe is not safe.
I don' know why the 2nd ' tmp = skb->next' in skb_queue_walk_safe
is needed and it may become unsafe if skb is freed during current loop.
But removing the 2nd 'tmp = skb->next' doesn't help the problem, because
tmp still may become freed after releasing lock.
> If its state is x_done/tx_done/rx_cleanup, that means the the skb will
> be released soon, right? If so, it should avoid calling
> usb_unlink_urb().
Even though you can avoid calling unlink for completed URBs, the skbs
still may be freed in unlink path because complete handler will be triggered
by unlink and the referenced skb may be freed before next loop, so your
patch can't fix the oops.
As far as I can think of, we can hold lock of done queue to forbid skb free
during unlinking. The below patch may fix the problem, are you OK
with it?
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index db99536..a9809d4 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -581,7 +581,8 @@ static int unlink_urbs (struct usbnet *dev, struct
sk_buff_head *q)
struct sk_buff *skb, *skbnext;
int count = 0;
- spin_lock_irqsave (&q->lock, flags);
+ spin_lock_irqsave(&dev->done.lock, flags);
+ spin_lock(&q->lock);
skb_queue_walk_safe(q, skb, skbnext) {
struct skb_data *entry;
struct urb *urb;
@@ -598,7 +599,7 @@ static int unlink_urbs (struct usbnet *dev, struct
sk_buff_head *q)
* handler(include defer_bh).
*/
usb_get_urb(urb);
- spin_unlock_irqrestore(&q->lock, flags);
+ spin_unlock(&q->lock);
// during some PM-driven resume scenarios,
// these (async) unlinks complete immediately
retval = usb_unlink_urb (urb);
@@ -607,9 +608,10 @@ static int unlink_urbs (struct usbnet *dev,
struct sk_buff_head *q)
else
count++;
usb_put_urb(urb);
- spin_lock_irqsave(&q->lock, flags);
+ spin_lock(&q->lock);
}
- spin_unlock_irqrestore (&q->lock, flags);
+ spin_unlock(&q->lock);
+ spin_unlock_irqrestore(&dev->done.lock, flags);
return count;
}
Thanks,
--
Ming Lei
^ permalink raw reply related
* Re: [PATCH v2] macvlan/macvtap: Fix vlan tagging on user read
From: Eric W. Biederman @ 2012-04-21 1:51 UTC (permalink / raw)
To: Basil Gor; +Cc: netdev, David S. Miller, Basil Gor
In-Reply-To: <1334964058-3338-1-git-send-email-basilgor@gmail.com>
Basil Gor <basil.gor@gmail.com> writes:
> Vlan tag is restored during buffer transmit to a network device (bridge
> port) in bridging code in case of tun/tap driver. In case of macvtap it
> has to be done explicitly. Otherwise vlan_tci is ignored and user always
> gets untagged packets.
>
> Scenario tested:
> kvm guests (that use vlans) migration from bridged network to macvtap
> revealed that packets delivered to guests are always untagged. Dumping
> and comparing sk_buff in case of tap and macvtap driver showed that
> macvtap does not restore vlan_tci.
>
> With current patch applied I was able to get working network, kvm guests
> get correctly tagged packets and can reach each other when macvtap in
> bridge mode (both with no vlans and through vlan interfaces).
>
> Changes from original version:
> vlan header restoring code is moved from macvtap_forward to
> macvtap_receive
I really think this is the wrong fix.
Eric
^ permalink raw reply
* Re: [PATCH] macvlan/macvtap: Fix vlan tagging on user read
From: Eric W. Biederman @ 2012-04-21 1:49 UTC (permalink / raw)
To: Basil Gor; +Cc: netdev, David S. Miller
In-Reply-To: <20120420231128.GA2088@nanobar>
Basil Gor <basil.gor@gmail.com> writes:
> I did some additional code review, and it's easier to show on stack traces and
> by comparing macvtap with tun/tap driver.
>
> tun/tap device does not need to care about vlan tag stuff, as it gets skb with
> vlan id in the header and vlan_tci is not used.
>
> [97493.070321] tun_net_xmit devname vnet0 vlan_tci 0 vlan 0 proto 8100 len 64
> [97493.070327] Pid: 0, comm: swapper/2 Tainted: G O 3.3.1-3.fc16.x86_64 #1
> [97493.070331] Call Trace:
> [97493.070334] <IRQ> [<ffffffffa02c6827>] tun_net_xmit+0x47/0x260 [tun]
> [97493.070347] [<ffffffff814e8072>] dev_hard_start_xmit+0x332/0x6d0 <------ __vlan_put_tag is called
> [97493.070355] [<ffffffff81503f5a>] sch_direct_xmit+0xfa/0x1d0
> [97493.070364] [<ffffffff814e85b5>] dev_queue_xmit+0x1a5/0x640
> [97493.070377] [<ffffffffa057c180>] ? br_flood+0xc0/0xc0 [bridge]
> [97493.070395] [<ffffffffa057c380>] ? __br_deliver+0x100/0x100 [bridge]
> [97493.070409] [<ffffffffa057c380>] ? __br_deliver+0x100/0x100 [bridge]
> [97493.070423] [<ffffffffa057c1ec>] br_dev_queue_push_xmit+0x6c/0xa0 [bridge]
> [97493.070438] [<ffffffffa057c380>] ? __br_deliver+0x100/0x100 [bridge]
> [97493.070457] [<ffffffffa057c242>] br_forward_finish+0x22/0x60 [bridge]
> [97493.070471] [<ffffffffa057c380>] ? __br_deliver+0x100/0x100 [bridge]
> [97493.070485] [<ffffffffa057c3dd>] __br_forward+0x5d/0xb0 [bridge]
> [97493.070495] [<ffffffff814dafe4>] ? skb_clone+0x54/0xb0
> [97493.070508] [<ffffffffa057bf1e>] deliver_clone+0x3e/0x60 [bridge]
> [97493.070523] [<ffffffffa057c143>] br_flood+0x83/0xc0 [bridge]
> [97493.070534] [<ffffffffa057c525>] br_flood_forward+0x15/0x20 [bridge]
> [97493.070544] [<ffffffffa057d256>] br_handle_frame_finish+0x246/0x2a0 [bridge]
> [97493.070555] [<ffffffffa057d444>] br_handle_frame+0x194/0x260 [bridge]
> [97493.070567] [<ffffffffa057d2b0>] ? br_handle_frame_finish+0x2a0/0x2a0 [bridge]
> [97493.070581] [<ffffffff814e56de>] __netif_receive_skb+0x1be/0x5c0 <------ vlan_untag is called
> [97493.070594] [<ffffffff81088ba2>] ? default_wake_function+0x12/0x20
> [97493.070604] [<ffffffff814e5ef1>] process_backlog+0xb1/0x170
> [97493.070613] [<ffffffff814e718b>] net_rx_action+0x12b/0x270
> [97493.070623] [<ffffffff8108daed>] ? sched_clock_cpu+0xbd/0x110
> [97493.070633] [<ffffffff8105efb8>] __do_softirq+0xb8/0x230
> [97493.070644] [<ffffffff810e3c30>] ? handle_irq_event+0x50/0x70
> [97493.070654] [<ffffffff815fd49c>] call_softirq+0x1c/0x30
> [97493.070662] [<ffffffff81016455>] do_softirq+0x65/0xa0
> [97493.070668] [<ffffffff8105f3ce>] irq_exit+0x9e/0xc0
> [97493.070675] [<ffffffff815fdd03>] do_IRQ+0x63/0xe0
> [97493.070682] [<ffffffff815f42ae>] common_interrupt+0x6e/0x6e
> [97493.070686] <EOI> [<ffffffff8131b236>] ? intel_idle+0xe6/0x150
> [97493.070697] [<ffffffff8131b218>] ? intel_idle+0xc8/0x150
> [97493.070705] [<ffffffff814a4071>] cpuidle_idle_call+0xc1/0x280
> [97493.070713] [<ffffffff8101322f>] cpu_idle+0xcf/0x120
> [97493.070720] [<ffffffff815e3b1f>] start_secondary+0x282/0x284
>
> but macvtap device gets skb with vlan tag extacted in vlan_tci, and as original driver
> code was mostly based on tun/tap driver vlan thing was missed.
>
> [98143.863560] macvtap_receive devname macvtap0 vlan_tci 100a vlan 10 proto 0800 len 90
> [98143.863570] macvtap_forward devname macvtap0 vlan_tci 100a vlan 10 proto 0800 len 90
> [98143.863578] Pid: 0, comm: swapper/2 Tainted: G O 3.3.1-3.fc16.x86_64 #1
> [98143.863583] Call Trace:
> [98143.863587] <IRQ> [<ffffffffa026819c>] macvtap_forward+0x8c/0x1b0 [macvtap]
> [98143.863606] [<ffffffffa0268314>] macvtap_receive+0x54/0x60 [macvtap]
> [98143.863623] [<ffffffffa02c05db>] macvlan_handle_frame+0xbb/0x2c0 [macvlan]
> [98143.863635] [<ffffffffa02c0520>] ? macvlan_broadcast+0x160/0x160 [macvlan]
> [98143.863646] [<ffffffff814e56de>] __netif_receive_skb+0x1be/0x5c0 <------ vlan_untag is called
> [98143.863653] [<ffffffff814e67e3>] netif_receive_skb+0x23/0x90
> [98143.863660] [<ffffffff814e6c09>] ? dev_gro_receive+0x1b9/0x2b0
> [98143.863667] [<ffffffff814e6950>] napi_skb_finish+0x50/0x70
> [98143.863673] [<ffffffff814e6f45>] napi_gro_receive+0xf5/0x140
> [98143.863697] [<ffffffffa0241fab>] e1000_receive_skb+0x5b/0x70 [e1000e]
Actually __vlan_hwaccel_put_tag is called here not vlan_untag
> [98143.863718] [<ffffffffa0244b21>] e1000_clean_rx_irq+0x2f1/0x400 [e1000e]
> [98143.863737] [<ffffffffa02432e8>] e1000_clean+0x78/0x2c0 [e1000e]
> [98143.863745] [<ffffffff814e718b>] net_rx_action+0x12b/0x270
> [98143.863752] [<ffffffff8108daed>] ? sched_clock_cpu+0xbd/0x110
> [98143.863759] [<ffffffff8105efb8>] __do_softirq+0xb8/0x230
> [98143.863767] [<ffffffff810e3c30>] ? handle_irq_event+0x50/0x70
> [98143.863775] [<ffffffff815fd49c>] call_softirq+0x1c/0x30
> [98143.863782] [<ffffffff81016455>] do_softirq+0x65/0xa0
> [98143.863788] [<ffffffff8105f3ce>] irq_exit+0x9e/0xc0
> [98143.863796] [<ffffffff815fdd03>] do_IRQ+0x63/0xe0
> [98143.863803] [<ffffffff815f42ae>] common_interrupt+0x6e/0x6e
> [98143.863807] <EOI> [<ffffffff8131b236>] ? intel_idle+0xe6/0x150
> [98143.863818] [<ffffffff8131b218>] ? intel_idle+0xc8/0x150
> [98143.863826] [<ffffffff814a4071>] cpuidle_idle_call+0xc1/0x280
> [98143.863834] [<ffffffff8101322f>] cpu_idle+0xcf/0x120
> [98143.863841] [<ffffffff815e3b1f>] start_secondary+0x282/0x284
>
> and as Eric Biederman noted, why not add vlan header back at the last moment? in
> macvtap_put_user. And it would work for user space applications which read
> /dev/tapX, but in kvm case actual reading is done by vhost_net driver. And this
> driver actually does skb_peek on macvtap queue to get next packet size before
> reading (in handle_rx).
>
> [98143.863878] vhost peek_head_len devname macvtap0 vlan_tci 100a vlan 10 proto 0800 len 90
>
> so, it gets skb len without vlan tag and then performs read with buffer smaller then needed
>
> [98143.863885] macvtap_do_read buflen 102 <--- 90 + (vnet_hdr_sz 12 bytes)
> [98143.863889] devname macvtap0 vlan_tci 100a vlan 10 proto 0800 len 90
> __vlan_put_tag is called here
> [98143.863894] macvtap_do_read reallen 106 <--- 90 + 4 + (vnet_hdr_sz 12)
> [98143.863898] devname macvtap0 vlan_tci 0 vlan 0 proto 8100 len 94
> [98143.863904] Pid: 7289, comm: vhost-7236 Tainted: G O 3.3.1-3.fc16.x86_64 #1
> [98143.863935] Call Trace:
> [98143.863944] [<ffffffffa0268593>] macvtap_do_read+0x243/0x420 [macvtap]
> [98143.863954] [<ffffffff81088b90>] ? try_to_wake_up+0x2b0/0x2b0
> [98143.863962] [<ffffffffa02687ba>] macvtap_recvmsg+0x4a/0x70 [macvtap]
> [98143.863971] [<ffffffffa02e149e>] handle_rx+0x39e/0x6e0 [vhost_net]
> [98143.863983] [<ffffffffa02e17f5>] handle_rx_net+0x15/0x20 [vhost_net]
> [98143.863996] [<ffffffffa02de84c>] vhost_worker+0xcc/0x150 [vhost_net]
> [98143.864008] [<ffffffffa02de780>] ? __vhost_add_used_n+0x110/0x110 [vhost_net]
> [98143.864020] [<ffffffff81079af3>] kthread+0x93/0xa0
> [98143.864032] [<ffffffff815fd3a4>] kernel_thread_helper+0x4/0x10
> [98143.864044] [<ffffffff81079a60>] ? kthread_freezable_should_stop+0x70/0x70
> [98143.864056] [<ffffffff815fd3a0>] ? gs_change+0x13/0x13
>
> things get more interesting when we take another case in account. When one kvm guest sends
> packet on the same macvlan to another guest macvtap gets skb with vlan id in the header
> and vlan_tci is not used.
>
> [99564.523943] macvtap_forward devname (null) vlan_tci 0 vlan 0 proto 8100 len 94
> [99564.523946] Pid: 8849, comm: vhost-8797 Tainted: G O 3.3.1-3.fc16.x86_64 #1
> [99564.523947] Call Trace:
> [99564.523952] [<ffffffffa02de19c>] macvtap_forward+0x8c/0x1b0 [macvtap]
> [99564.523963] [<ffffffffa02c0502>] macvlan_broadcast+0x142/0x160 [macvlan]
> [99564.523967] [<ffffffffa02c146d>] macvlan_start_xmit+0x14d/0x178 [macvlan]
> [99564.523969] [<ffffffffa02df378>] macvtap_get_user+0x388/0x420 [macvtap]
> [99564.523971] [<ffffffffa02df43b>] macvtap_sendmsg+0x2b/0x30 [macvtap]
> [99564.523973] [<ffffffffa026bb3d>] handle_tx+0x2dd/0x620 [vhost_net]
> [99564.523976] [<ffffffffa026beb5>] handle_tx_kick+0x15/0x20 [vhost_net]
> [99564.523978] [<ffffffffa026884c>] vhost_worker+0xcc/0x150 [vhost_net]
> [99564.523980] [<ffffffffa0268780>] ? __vhost_add_used_n+0x110/0x110 [vhost_net]
> [99564.523984] [<ffffffff81079af3>] kthread+0x93/0xa0
> [99564.523987] [<ffffffff815fd3a4>] kernel_thread_helper+0x4/0x10
> [99564.523989] [<ffffffff81079a60>] ? kthread_freezable_should_stop+0x70/0x70
> [99564.523991] [<ffffffff815fd3a0>] ? gs_change+0x13/0x13
> [99564.523999] vhost peek_head_len devname (null) vlan_tci 0 vlan 0 proto 8100 len 94
> [99564.524003] macvtap_do_read buflen 106
> [99564.524004] macvtap_do_read reallen 106
>
> And we definitely want to have common rules for all cases. So we either
> 1. restore vlan headers from vlan_tci for any packets coming outside of macvlan in
> macvtap_receive and we don't need to fix vhost_net and we preserve same vlan id
> policy that tun/tap driver have. (my original patch)
> or
> 2. we extract vlan ids for packets coming from the same macvlan, fixing vhost_net to
> take vlan_tci into account and restoring vlan headers on
> macvtap_put_user
And 2 seems to be the right answer.
Not long ago we went a few rounds with this with the core parts of the
networking stack and the rule that evolved was that we always strip the
vlan tag. Which is why we have the vlan_untag call in __netif_receive_skb()
to handle the small handful of networking devices that don't.
vhost/net.c is just buggy. PF_PACKET sockets have been returning
the vlan_id in ancillary data from year hardware like the e1000 since
at least 2005.
So I agree that both macvtap and vhost/net.c both need to be fixed.
Eric
^ permalink raw reply
* Re: [PATCH net-next 00/19] net: Sysctl simplifications and enhancements
From: David Miller @ 2012-04-21 1:24 UTC (permalink / raw)
To: xemul; +Cc: ebiederm, netdev, serge, gaofeng, pablo, shemminger
In-Reply-To: <4F912240.8010802@parallels.com>
From: Pavel Emelyanov <xemul@parallels.com>
Date: Fri, 20 Apr 2012 12:45:52 +0400
> On 04/20/2012 03:17 AM, Eric W. Biederman wrote:
>>
>> Summary:
>> - Kill approximately 400 lines of code
>> - Allow all networking sysctls with just CAP_NET_ADMIN
>> - Hide all networking sysctls that don't apply to your current network namespace.
>> - Uniformly register flat sysctl tables not sysctl tables with .child entries
>> - Readable string paths for registering sysctls
>>
>> Eric W. Biederman (19):
>> net: Implement register_net_sysctl.
>> net sysctl: Register an empty /proc/sys/net
>> net sysctl: Initialize the network sysctls sooner to avoid problems.
>> net: Kill register_sysctl_rotable
>> net: Move all of the network sysctls without a namespace into init_net.
>> net core: Remove unneded creation of an empty net/core sysctl directory
>> net ipv6: Remove unneded registration of an empty net/ipv6/neigh
>> net ipv4: Remove the unneeded registration of an empty net/ipv4/neigh
>> net ax25: Simplify and cleanup the ax25 sysctl handling.
>> net llc: Don't use sysctl tables with .child entries.
>> net ipv6: Don't use sysctl tables with .child entries.
>> net neighbour: Convert to use register_net_sysctl
>> net decnet: Convert to use register_net_sysctl
>> net ipv6: Convert addrconf to use register_net_sysctl
>> net ipv4: Convert devinet to use register_net_sysctl
>> net: Convert nf_conntrack_proto to use register_net_sysctl
>> net: Convert all sysctl registrations to register_net_sysctl
>> net: Delete all remaining instances of ctl_path
>> net: Remove register_net_sysctl_table
>
> After resolving issues with Eric
>
> Acked-by: Pavel Emelyanov <xemul@parallels.com>
Series applied, thanks everyone.
^ permalink raw reply
* Re: [PATCH v3.4-rc 0/9] pch_gbe: ptp bug fixes
From: David Miller @ 2012-04-21 0:50 UTC (permalink / raw)
To: richardcochran; +Cc: netdev, tshimizu818
In-Reply-To: <cover.1334949388.git.richardcochran@gmail.com>
From: Richard Cochran <richardcochran@gmail.com>
Date: Fri, 20 Apr 2012 22:09:19 +0200
> The content in this patch series originally was posted to netdev by
> Takahiro on March 23, 2012, as a loose group of five patches. These
> patches addressed problems pointed out in review, both on and off
> list.
>
> I have taken his work and rebased, squashed, and split it into the
> present form, and I also wrote commit messages that more fully explain
> the changes. Also, I added the last two patches myself.
3.4-rcX is not the appropriate tree to base this work on, whereas
net-next is.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next 00/16] tipc: publication lists and zero node handling
From: David Miller @ 2012-04-21 0:45 UTC (permalink / raw)
To: paul.gortmaker; +Cc: netdev, allan.stephens, ying.xue
In-Reply-To: <1334955924-907-1-git-send-email-paul.gortmaker@windriver.com>
From: Paul Gortmaker <paul.gortmaker@windriver.com>
Date: Fri, 20 Apr 2012 17:05:08 -0400
> This series of commits largely focuses on two things. The 1st is
> to categorize the TIPC publication lists, so better control over
> the publications can be achieved. The 2nd is largely to improve
> corner cases around the tipc address changes and how the <0.0.0>
> tipc address is interpreted.
>
> What remains after this, is less than 10 commits originating from
> what was salvaged from the tipc-1.7 sourceforge fork, so I'm happy to
> see the end of this task so close. I'll be looking at those shortly.
>
> The tipc tests (tipcTS <--> tipcTC) have been used in both directions
> between a 32bit and a 64bit machine to do runtime testing, and the
> entire series has been confirmed to be compile time bisectable too.
>
> Credit to Al for all the heavy lifting. I've just refactored things
> a bit and made minor changes here and there.
Pulled, thanks Paul.
Please remove the inline tags from static functions, let the compiler
decide what to do. The only spot appropriate to use inline tags these
days is helper functions defined in header files.
^ permalink raw reply
* Re: [PATCHv2] workqueue: Catch more locking problems with flush_work()
From: Yong Zhang @ 2012-04-21 0:34 UTC (permalink / raw)
To: Stephen Boyd; +Cc: Tejun Heo, linux-kernel, netdev, Ben Dooks
In-Reply-To: <1334968130-20724-1-git-send-email-sboyd@codeaurora.org>
On Sat, Apr 21, 2012 at 8:28 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> If a workqueue is flushed with flush_work() lockdep checking can
> be circumvented. For example:
>
> static DEFINE_MUTEX(mutex);
>
> static void my_work(struct work_struct *w)
> {
> mutex_lock(&mutex);
> mutex_unlock(&mutex);
> }
>
> static DECLARE_WORK(work, my_work);
>
> static int __init start_test_module(void)
> {
> schedule_work(&work);
> return 0;
> }
> module_init(start_test_module);
>
> static void __exit stop_test_module(void)
> {
> mutex_lock(&mutex);
> flush_work(&work);
> mutex_unlock(&mutex);
> }
> module_exit(stop_test_module);
>
> would not always print a warning when flush_work() was called.
> In this trivial example nothing could go wrong since we are
> guaranteed module_init() and module_exit() don't run concurrently,
> but if the work item is schedule asynchronously we could have a
> scenario where the work item is running just at the time flush_work()
> is called resulting in a classic ABBA locking problem.
>
> Add a lockdep hint by acquiring and releasing the work item
> lockdep_map in flush_work() so that we always catch this
> potential deadlock scenario.
>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
Reviewed-by: Yong Zhang <yong.zhang0@gmail.com>
> ---
> kernel/workqueue.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 5abf42f..038cf64 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2506,6 +2506,9 @@ bool flush_work(struct work_struct *work)
> {
> struct wq_barrier barr;
>
> + lock_map_acquire(&work->lockdep_map);
> + lock_map_release(&work->lockdep_map);
> +
> if (start_flush_work(work, &barr, true)) {
> wait_for_completion(&barr.done);
> destroy_work_on_stack(&barr.work);
> --
> Sent by an employee of the Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
>
--
Only stand for myself
^ permalink raw reply
* Re: [PATCH 1/2] workqueue: Catch more locking problems with flush_work()
From: Yong Zhang @ 2012-04-21 0:32 UTC (permalink / raw)
To: Stephen Boyd; +Cc: linux-kernel, Tejun Heo, netdev, Ben Dooks
In-Reply-To: <20120420083202.GB17846@zhy>
On Fri, Apr 20, 2012 at 4:32 PM, Yong Zhang <yong.zhang0@gmail.com> wrote:
> On Fri, Apr 20, 2012 at 01:18:19AM -0700, Stephen Boyd wrote:
>> On 4/20/2012 12:18 AM, Yong Zhang wrote:
>> > On Thu, Apr 19, 2012 at 11:26:47PM -0700, Stephen Boyd wrote:
>> >> complain in the case where the work is not queued. That case is not a
>> >> false positive. We will get a lockdep warning if the work is running
>> > IIRC, flush_work() is just a nop when a work is not queued nor running.
>>
>> Agreed, but it's better to always print a lockdep warning instead of
>> only when the deadlock is going to occur.
>
> It will IMHO.
After reconsidering this issue, I recognize I'm wrong from the beginning.
My suggestion will only warn on the real deadlock. It doesn't follow
what lockdep want to achieve. Sorry for that.
BTW, your latest patch should work :)
Thanks,
Yong
>
>>
>> >
>> >> (when start_flush_work() returns true) solely with the
>> >> lock_map_acquire() on cwq->wq->lockdep_map.
>> > Yeah, that is the point we use lockdep to detect deadlock for workqueue.
>> >
>> > But when looking at start_flush_work(), for some case
>> > !(cwq->wq->saved_max_active == 1 || cwq->wq->flags & WQ_RESCUER),
>> > lock_map_acquire_read() is called, but recursive read is not added to
>> > the chain list. So when lock_map_acquire_read(&cwq->wq->lockdep_map)
>> > is called, deadlock will not be detected. I hope you don't hit that
>> > special case.
>>
>> Hmm. Originally I had what you suggested in my patch but I left it out
>> because I wasn't sure if it would cause false positives.
>> Do you see any
>> possibility for false positives?
>
> No, I don't. My test indeed show nothing (just build and boot).
>
>>I'll add it back in if not.
>
> It's great if you can try it :)
>
> Thanks,
> Yong
--
Only stand for myself
^ permalink raw reply
* [PATCHv2] workqueue: Catch more locking problems with flush_work()
From: Stephen Boyd @ 2012-04-21 0:28 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-kernel, netdev, Ben Dooks, Yong Zhang
In-Reply-To: <20120420173529.GD32324@google.com>
If a workqueue is flushed with flush_work() lockdep checking can
be circumvented. For example:
static DEFINE_MUTEX(mutex);
static void my_work(struct work_struct *w)
{
mutex_lock(&mutex);
mutex_unlock(&mutex);
}
static DECLARE_WORK(work, my_work);
static int __init start_test_module(void)
{
schedule_work(&work);
return 0;
}
module_init(start_test_module);
static void __exit stop_test_module(void)
{
mutex_lock(&mutex);
flush_work(&work);
mutex_unlock(&mutex);
}
module_exit(stop_test_module);
would not always print a warning when flush_work() was called.
In this trivial example nothing could go wrong since we are
guaranteed module_init() and module_exit() don't run concurrently,
but if the work item is schedule asynchronously we could have a
scenario where the work item is running just at the time flush_work()
is called resulting in a classic ABBA locking problem.
Add a lockdep hint by acquiring and releasing the work item
lockdep_map in flush_work() so that we always catch this
potential deadlock scenario.
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
kernel/workqueue.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 5abf42f..038cf64 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2506,6 +2506,9 @@ bool flush_work(struct work_struct *work)
{
struct wq_barrier barr;
+ lock_map_acquire(&work->lockdep_map);
+ lock_map_release(&work->lockdep_map);
+
if (start_flush_work(work, &barr, true)) {
wait_for_completion(&barr.done);
destroy_work_on_stack(&barr.work);
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply related
* restoring IP multicast addresses when restarting the interface.
From: Flavio Leitner @ 2012-04-21 0:25 UTC (permalink / raw)
To: netdev; +Cc: Herbert Xu
Hello folks,
I was told that ntp in multicast mode was not working if you
restart the interface (ifdown <iface> ; ifup <iface) on an
older kernel.
It seemed obvious to me, but when I tried to reproduce this
upstream (last kernel of today), I noticed that the kernel
does restore the previous added multicast addresses. Therefore,
there is no issues.
I found that this is due to mainly this commit below which
doesn't remove idev when the addresses are deleted.
commit 6363097cc4d182f93788131b5d8f72aa91d950a0
Author: Herbert Xu <herbert@gondor.apana.org.au>
Date: Thu Jun 7 18:35:38 2007 -0700
[IPV4]: Do not remove idev when addresses are cleared
Now that we create idev before addresses are added, it no longer makes
sense to remove them when addresses are all deleted.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: David S. Miller <davem@davemloft.net>
Although the new behavior seems nice and save some user space
work, I think it was unintentional and likely to be a bug.
What you guys think?
thanks,
fbl
^ permalink raw reply
* [PATCH v2] macvlan/macvtap: Fix vlan tagging on user read
From: Basil Gor @ 2012-04-20 23:20 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Eric W. Biederman, Basil Gor
Vlan tag is restored during buffer transmit to a network device (bridge
port) in bridging code in case of tun/tap driver. In case of macvtap it
has to be done explicitly. Otherwise vlan_tci is ignored and user always
gets untagged packets.
Scenario tested:
kvm guests (that use vlans) migration from bridged network to macvtap
revealed that packets delivered to guests are always untagged. Dumping
and comparing sk_buff in case of tap and macvtap driver showed that
macvtap does not restore vlan_tci.
With current patch applied I was able to get working network, kvm guests
get correctly tagged packets and can reach each other when macvtap in
bridge mode (both with no vlans and through vlan interfaces).
Changes from original version:
vlan header restoring code is moved from macvtap_forward to
macvtap_receive
Signed-off-by: Basil Gor <basilgor@gmail.com>
---
drivers/net/macvtap.c | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 0427c65..5e025d9 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -1,6 +1,7 @@
#include <linux/etherdevice.h>
#include <linux/if_macvlan.h>
#include <linux/interrupt.h>
+#include <linux/if_vlan.h>
#include <linux/nsproxy.h>
#include <linux/compat.h>
#include <linux/if_tun.h>
@@ -271,6 +272,15 @@ drop:
static int macvtap_receive(struct sk_buff *skb)
{
skb_push(skb, ETH_HLEN);
+
+ if (vlan_tx_tag_present(skb)) {
+ skb = __vlan_put_tag(skb, vlan_tx_tag_get(skb));
+ if (unlikely(!skb))
+ return NET_RX_DROP;
+
+ skb->vlan_tci = 0;
+ }
+
return macvtap_forward(skb->dev, skb);
}
--
1.7.6.5
^ permalink raw reply related
* Re: [PATCH 1/2] workqueue: Catch more locking problems with flush_work()
From: Stephen Boyd @ 2012-04-20 23:15 UTC (permalink / raw)
To: Tejun Heo; +Cc: linux-kernel, netdev, Ben Dooks
In-Reply-To: <20120420173529.GD32324@google.com>
On 4/20/2012 10:35 AM, Tejun Heo wrote:
>
> All that's necessary is acquiring and releasing work->lockdep_map.
> There's no need to nest start_flush_work() inside it. Without
> nesting, there's nothing to worry about ABBA lockdeps.
Yes I see. This patch should work then?
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 5abf42f..038cf64 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2506,6 +2506,9 @@ bool flush_work(struct work_struct *work)
{
struct wq_barrier barr;
+ lock_map_acquire(&work->lockdep_map);
+ lock_map_release(&work->lockdep_map);
+
if (start_flush_work(work, &barr, true)) {
wait_for_completion(&barr.done);
destroy_work_on_stack(&barr.work);
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply related
* Re: [PATCH] macvlan/macvtap: Fix vlan tagging on user read
From: Basil Gor @ 2012-04-20 23:11 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: netdev, David S. Miller
In-Reply-To: <20120418193312.GA24516@nanobar>
I did some additional code review, and it's easier to show on stack traces and
by comparing macvtap with tun/tap driver.
tun/tap device does not need to care about vlan tag stuff, as it gets skb with
vlan id in the header and vlan_tci is not used.
[97493.070321] tun_net_xmit devname vnet0 vlan_tci 0 vlan 0 proto 8100 len 64
[97493.070327] Pid: 0, comm: swapper/2 Tainted: G O 3.3.1-3.fc16.x86_64 #1
[97493.070331] Call Trace:
[97493.070334] <IRQ> [<ffffffffa02c6827>] tun_net_xmit+0x47/0x260 [tun]
[97493.070347] [<ffffffff814e8072>] dev_hard_start_xmit+0x332/0x6d0 <------ __vlan_put_tag is called
[97493.070355] [<ffffffff81503f5a>] sch_direct_xmit+0xfa/0x1d0
[97493.070364] [<ffffffff814e85b5>] dev_queue_xmit+0x1a5/0x640
[97493.070377] [<ffffffffa057c180>] ? br_flood+0xc0/0xc0 [bridge]
[97493.070395] [<ffffffffa057c380>] ? __br_deliver+0x100/0x100 [bridge]
[97493.070409] [<ffffffffa057c380>] ? __br_deliver+0x100/0x100 [bridge]
[97493.070423] [<ffffffffa057c1ec>] br_dev_queue_push_xmit+0x6c/0xa0 [bridge]
[97493.070438] [<ffffffffa057c380>] ? __br_deliver+0x100/0x100 [bridge]
[97493.070457] [<ffffffffa057c242>] br_forward_finish+0x22/0x60 [bridge]
[97493.070471] [<ffffffffa057c380>] ? __br_deliver+0x100/0x100 [bridge]
[97493.070485] [<ffffffffa057c3dd>] __br_forward+0x5d/0xb0 [bridge]
[97493.070495] [<ffffffff814dafe4>] ? skb_clone+0x54/0xb0
[97493.070508] [<ffffffffa057bf1e>] deliver_clone+0x3e/0x60 [bridge]
[97493.070523] [<ffffffffa057c143>] br_flood+0x83/0xc0 [bridge]
[97493.070534] [<ffffffffa057c525>] br_flood_forward+0x15/0x20 [bridge]
[97493.070544] [<ffffffffa057d256>] br_handle_frame_finish+0x246/0x2a0 [bridge]
[97493.070555] [<ffffffffa057d444>] br_handle_frame+0x194/0x260 [bridge]
[97493.070567] [<ffffffffa057d2b0>] ? br_handle_frame_finish+0x2a0/0x2a0 [bridge]
[97493.070581] [<ffffffff814e56de>] __netif_receive_skb+0x1be/0x5c0 <------ vlan_untag is called
[97493.070594] [<ffffffff81088ba2>] ? default_wake_function+0x12/0x20
[97493.070604] [<ffffffff814e5ef1>] process_backlog+0xb1/0x170
[97493.070613] [<ffffffff814e718b>] net_rx_action+0x12b/0x270
[97493.070623] [<ffffffff8108daed>] ? sched_clock_cpu+0xbd/0x110
[97493.070633] [<ffffffff8105efb8>] __do_softirq+0xb8/0x230
[97493.070644] [<ffffffff810e3c30>] ? handle_irq_event+0x50/0x70
[97493.070654] [<ffffffff815fd49c>] call_softirq+0x1c/0x30
[97493.070662] [<ffffffff81016455>] do_softirq+0x65/0xa0
[97493.070668] [<ffffffff8105f3ce>] irq_exit+0x9e/0xc0
[97493.070675] [<ffffffff815fdd03>] do_IRQ+0x63/0xe0
[97493.070682] [<ffffffff815f42ae>] common_interrupt+0x6e/0x6e
[97493.070686] <EOI> [<ffffffff8131b236>] ? intel_idle+0xe6/0x150
[97493.070697] [<ffffffff8131b218>] ? intel_idle+0xc8/0x150
[97493.070705] [<ffffffff814a4071>] cpuidle_idle_call+0xc1/0x280
[97493.070713] [<ffffffff8101322f>] cpu_idle+0xcf/0x120
[97493.070720] [<ffffffff815e3b1f>] start_secondary+0x282/0x284
but macvtap device gets skb with vlan tag extacted in vlan_tci, and as original driver
code was mostly based on tun/tap driver vlan thing was missed.
[98143.863560] macvtap_receive devname macvtap0 vlan_tci 100a vlan 10 proto 0800 len 90
[98143.863570] macvtap_forward devname macvtap0 vlan_tci 100a vlan 10 proto 0800 len 90
[98143.863578] Pid: 0, comm: swapper/2 Tainted: G O 3.3.1-3.fc16.x86_64 #1
[98143.863583] Call Trace:
[98143.863587] <IRQ> [<ffffffffa026819c>] macvtap_forward+0x8c/0x1b0 [macvtap]
[98143.863606] [<ffffffffa0268314>] macvtap_receive+0x54/0x60 [macvtap]
[98143.863623] [<ffffffffa02c05db>] macvlan_handle_frame+0xbb/0x2c0 [macvlan]
[98143.863635] [<ffffffffa02c0520>] ? macvlan_broadcast+0x160/0x160 [macvlan]
[98143.863646] [<ffffffff814e56de>] __netif_receive_skb+0x1be/0x5c0 <------ vlan_untag is called
[98143.863653] [<ffffffff814e67e3>] netif_receive_skb+0x23/0x90
[98143.863660] [<ffffffff814e6c09>] ? dev_gro_receive+0x1b9/0x2b0
[98143.863667] [<ffffffff814e6950>] napi_skb_finish+0x50/0x70
[98143.863673] [<ffffffff814e6f45>] napi_gro_receive+0xf5/0x140
[98143.863697] [<ffffffffa0241fab>] e1000_receive_skb+0x5b/0x70 [e1000e]
[98143.863718] [<ffffffffa0244b21>] e1000_clean_rx_irq+0x2f1/0x400 [e1000e]
[98143.863737] [<ffffffffa02432e8>] e1000_clean+0x78/0x2c0 [e1000e]
[98143.863745] [<ffffffff814e718b>] net_rx_action+0x12b/0x270
[98143.863752] [<ffffffff8108daed>] ? sched_clock_cpu+0xbd/0x110
[98143.863759] [<ffffffff8105efb8>] __do_softirq+0xb8/0x230
[98143.863767] [<ffffffff810e3c30>] ? handle_irq_event+0x50/0x70
[98143.863775] [<ffffffff815fd49c>] call_softirq+0x1c/0x30
[98143.863782] [<ffffffff81016455>] do_softirq+0x65/0xa0
[98143.863788] [<ffffffff8105f3ce>] irq_exit+0x9e/0xc0
[98143.863796] [<ffffffff815fdd03>] do_IRQ+0x63/0xe0
[98143.863803] [<ffffffff815f42ae>] common_interrupt+0x6e/0x6e
[98143.863807] <EOI> [<ffffffff8131b236>] ? intel_idle+0xe6/0x150
[98143.863818] [<ffffffff8131b218>] ? intel_idle+0xc8/0x150
[98143.863826] [<ffffffff814a4071>] cpuidle_idle_call+0xc1/0x280
[98143.863834] [<ffffffff8101322f>] cpu_idle+0xcf/0x120
[98143.863841] [<ffffffff815e3b1f>] start_secondary+0x282/0x284
and as Eric Biederman noted, why not add vlan header back at the last moment? in
macvtap_put_user. And it would work for user space applications which read
/dev/tapX, but in kvm case actual reading is done by vhost_net driver. And this
driver actually does skb_peek on macvtap queue to get next packet size before
reading (in handle_rx).
[98143.863878] vhost peek_head_len devname macvtap0 vlan_tci 100a vlan 10 proto 0800 len 90
so, it gets skb len without vlan tag and then performs read with buffer smaller then needed
[98143.863885] macvtap_do_read buflen 102 <--- 90 + (vnet_hdr_sz 12 bytes)
[98143.863889] devname macvtap0 vlan_tci 100a vlan 10 proto 0800 len 90
__vlan_put_tag is called here
[98143.863894] macvtap_do_read reallen 106 <--- 90 + 4 + (vnet_hdr_sz 12)
[98143.863898] devname macvtap0 vlan_tci 0 vlan 0 proto 8100 len 94
[98143.863904] Pid: 7289, comm: vhost-7236 Tainted: G O 3.3.1-3.fc16.x86_64 #1
[98143.863935] Call Trace:
[98143.863944] [<ffffffffa0268593>] macvtap_do_read+0x243/0x420 [macvtap]
[98143.863954] [<ffffffff81088b90>] ? try_to_wake_up+0x2b0/0x2b0
[98143.863962] [<ffffffffa02687ba>] macvtap_recvmsg+0x4a/0x70 [macvtap]
[98143.863971] [<ffffffffa02e149e>] handle_rx+0x39e/0x6e0 [vhost_net]
[98143.863983] [<ffffffffa02e17f5>] handle_rx_net+0x15/0x20 [vhost_net]
[98143.863996] [<ffffffffa02de84c>] vhost_worker+0xcc/0x150 [vhost_net]
[98143.864008] [<ffffffffa02de780>] ? __vhost_add_used_n+0x110/0x110 [vhost_net]
[98143.864020] [<ffffffff81079af3>] kthread+0x93/0xa0
[98143.864032] [<ffffffff815fd3a4>] kernel_thread_helper+0x4/0x10
[98143.864044] [<ffffffff81079a60>] ? kthread_freezable_should_stop+0x70/0x70
[98143.864056] [<ffffffff815fd3a0>] ? gs_change+0x13/0x13
things get more interesting when we take another case in account. When one kvm guest sends
packet on the same macvlan to another guest macvtap gets skb with vlan id in the header
and vlan_tci is not used.
[99564.523943] macvtap_forward devname (null) vlan_tci 0 vlan 0 proto 8100 len 94
[99564.523946] Pid: 8849, comm: vhost-8797 Tainted: G O 3.3.1-3.fc16.x86_64 #1
[99564.523947] Call Trace:
[99564.523952] [<ffffffffa02de19c>] macvtap_forward+0x8c/0x1b0 [macvtap]
[99564.523963] [<ffffffffa02c0502>] macvlan_broadcast+0x142/0x160 [macvlan]
[99564.523967] [<ffffffffa02c146d>] macvlan_start_xmit+0x14d/0x178 [macvlan]
[99564.523969] [<ffffffffa02df378>] macvtap_get_user+0x388/0x420 [macvtap]
[99564.523971] [<ffffffffa02df43b>] macvtap_sendmsg+0x2b/0x30 [macvtap]
[99564.523973] [<ffffffffa026bb3d>] handle_tx+0x2dd/0x620 [vhost_net]
[99564.523976] [<ffffffffa026beb5>] handle_tx_kick+0x15/0x20 [vhost_net]
[99564.523978] [<ffffffffa026884c>] vhost_worker+0xcc/0x150 [vhost_net]
[99564.523980] [<ffffffffa0268780>] ? __vhost_add_used_n+0x110/0x110 [vhost_net]
[99564.523984] [<ffffffff81079af3>] kthread+0x93/0xa0
[99564.523987] [<ffffffff815fd3a4>] kernel_thread_helper+0x4/0x10
[99564.523989] [<ffffffff81079a60>] ? kthread_freezable_should_stop+0x70/0x70
[99564.523991] [<ffffffff815fd3a0>] ? gs_change+0x13/0x13
[99564.523999] vhost peek_head_len devname (null) vlan_tci 0 vlan 0 proto 8100 len 94
[99564.524003] macvtap_do_read buflen 106
[99564.524004] macvtap_do_read reallen 106
And we definitely want to have common rules for all cases. So we either
1. restore vlan headers from vlan_tci for any packets coming outside of macvlan in
macvtap_receive and we don't need to fix vhost_net and we preserve same vlan id
policy that tun/tap driver have. (my original patch)
or
2. we extract vlan ids for packets coming from the same macvlan, fixing vhost_net to
take vlan_tci into account and restoring vlan headers on macvtap_put_user
or please propose another solution.
Basil Gor
On Wed, Apr 18, 2012 at 11:33:12PM +0400, Basil Gor wrote:
> On Wed, Apr 18, 2012 at 11:54:52AM -0700, Eric W. Biederman wrote:
> > Basil Gor <basilgor@gmail.com> writes:
> >
> > > Vlan tag is restored during buffer transmit to a network device (bridge
> > > port) in bridging code in case of tun/tap driver. In case of macvtap it
> > > has to be done explicitly. Otherwise vlan_tci is ignored and user always
> > > gets untagged packets.
> > >
> > > Scenario tested:
> > > kvm guests (that use vlans) migration from bridged network to macvtap
> > > revealed that packets delivered to guests are always untagged. Dumping
> > > and comparing sk_buff in case of tap and macvtap driver showed that
> > > macvtap does not restore vlan_tci.
> > >
> > > With current patch applied I was able to get working network, kvm guests
> > > get correctly tagged packets and can reach each other when macvtap in
> > > bridge mode (both with no vlans and through vlan interfaces).
> >
> > My first impression is that this is the wrong place to add a vlan
> > header back.
> >
> > You need to keep the vlan information in vlan_tci until just
> > before the packet is delivered to userspace. Which would suggest
> > the best place for these games is macvtap_put_user.
> >
> > Elsewhere vlan headers should not be explicitly stored in the packet.
> >
> > At least that was the rule last I looked.
> >
> > Eric
> >
> >
> This sounds right, and macvtap_put_user was the first place where I
> put vlan header adding. But qemu-kvm does smth like get pending data
> size and then read, and when I put code in macvtap_put_user qemu
> supplied buffer 4 bytes smaller then needed and packets were
> truncated. On the other hand tun/tap driver never keeps vlan info in
> vlan_tci because you can't do any vlan operations on it I think. So I
> decided to restore vlan header just before adding it to macvtap queue.
>
> But I'll try to look deeper in it.
> Thanks
> > > Signed-off-by: Basil Gor <basilgor@gmail.com>
> > > ---
> > > drivers/net/macvtap.c | 9 +++++++++
> > > 1 files changed, 9 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
> > > index 0427c65..a6802b9 100644
> > > --- a/drivers/net/macvtap.c
> > > +++ b/drivers/net/macvtap.c
> > > @@ -1,6 +1,7 @@
> > > #include <linux/etherdevice.h>
> > > #include <linux/if_macvlan.h>
> > > #include <linux/interrupt.h>
> > > +#include <linux/if_vlan.h>
> > > #include <linux/nsproxy.h>
> > > #include <linux/compat.h>
> > > #include <linux/if_tun.h>
> > > @@ -254,6 +255,14 @@ static int macvtap_forward(struct net_device *dev, struct sk_buff *skb)
> > > if (skb_queue_len(&q->sk.sk_receive_queue) >= dev->tx_queue_len)
> > > goto drop;
> > >
> > > + if (vlan_tx_tag_present(skb)) {
> > > + skb = __vlan_put_tag(skb, vlan_tx_tag_get(skb));
> > > + if (unlikely(!skb))
> > > + return NET_RX_DROP;
> > > +
> > > + skb->vlan_tci = 0;
> > > + }
> > > +
> > > skb_queue_tail(&q->sk.sk_receive_queue, skb);
> > > wake_up_interruptible_poll(sk_sleep(&q->sk), POLLIN | POLLRDNORM | POLLRDBAND);
> > > return NET_RX_SUCCESS;
^ permalink raw reply
* Re: e1000e tx queue timeout in 3.3.0 (bisected to BQL support for e1000e)
From: Ben Greear @ 2012-04-20 21:56 UTC (permalink / raw)
To: Tom Herbert; +Cc: John Fastabend, netdev, e1000-devel list
In-Reply-To: <CA+mtBx-K6-O3ea34kmn4LXtx73FqCHF16w7pBSq1fHNN+-DxCA@mail.gmail.com>
On 04/20/2012 02:21 PM, Tom Herbert wrote:
> Thanks John for pointers to those. Ben, are you running a kernel with
> these patches?
I just tested this on my e1000e and igb machine. With these patches,
I can no longer reproduce the problem.
So, please make sure those are queued up for 3.3 stable!
Thanks,
Ben
>
> Tom
>
>>
>> Tom, did you see these two patches? Maybe this is resolved by
>> the second patch.
>>
>> We needed these to fixup ixgbe and igb (i didn't test e1000e)
>> looks like we might want to push these at stable. I don't
>> believe they are in 3.3.
>>
>> commit b37c0fbe3f6dfba1f8ad2aed47fb40578a254635
>> Author: Alexander Duyck<alexander.h.duyck@intel.com>
>> Date: Tue Feb 7 02:29:06 2012 +0000
>>
>> net: Add memory barriers to prevent possible race in byte queue limits
>>
>> This change adds a memory barrier to the byte queue limit code to address a
>> possible race as has been seen in the past with the
>> netif_stop_queue/netif_wake_queue logic.
>>
>> Signed-off-by: Alexander Duyck<alexander.h.duyck@intel.com>
>> Tested-by: Stephen Ko<stephen.s.ko@intel.com>
>> Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>>
>>
>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=b37c0fbe3f6dfba1f8ad2aed47fb40578a254635
>>
>>
>> commit 5c4903549c05bbb373479e0ce2992573c120654a
>> Author: Alexander Duyck<alexander.h.duyck@intel.com>
>> Date: Tue Feb 7 02:29:01 2012 +0000
>>
>> net: Fix issue with netdev_tx_reset_queue not resetting queue from XOFF state
>>
>> We are seeing dev_watchdog hangs on several drivers. I suspect this is due
>> to the __QUEUE_STATE_STACK_XOFF bit being set prior to a reset for link
>> change, and then not being cleared by netdev_tx_reset_queue. This change
>> corrects that.
>>
>> In addition we were seeing dev_watchdog hangs on igb after running the
>> ethtool tests. We found this to be due to the fact that the ethtool test
>> runs the same logic as ndo_start_xmit, but we were never clearing the XOFF
>> flag since the loopback test in ethtool does not do byte queue accounting.
>>
>> Signed-off-by: Alexander Duyck<alexander.h.duyck@intel.com>
>> Tested-by: Stephen Ko<stephen.s.ko@intel.com>
>> Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>>
>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5c4903549c05bbb373479e0ce2992573c120654a
>>
>>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
------------------------------------------------------------------------------
For Developers, A Lot Can Happen In A Second.
Boundary is the first to Know...and Tell You.
Monitor Your Applications in Ultra-Fine Resolution. Try it FREE!
http://p.sf.net/sfu/Boundary-d2dvs2
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Hello Dear
From: Mis Tuma Ahoussou @ 2012-04-20 21:45 UTC (permalink / raw)
Hello Dear
How are you doing i hope
you are in a good health?
i hope to hear from you soon.
My Regards
Tuma.
^ permalink raw reply
* Re: [net-next 04/14] net: Fix issue with netdev_tx_reset_queue not resetting queue from XOFF state
From: John Fastabend @ 2012-04-20 21:31 UTC (permalink / raw)
To: Tom Herbert, Alexander Duyck; +Cc: Jeff Kirsher, davem, netdev, gospo, sassmann
In-Reply-To: <CA+mtBx-BWy=KANgSca9f2vxuu1Km_kPMJbQFa+Nq9q_YYvatBQ@mail.gmail.com>
On 4/20/2012 2:19 PM, Tom Herbert wrote:
> Hi Jeff,
>
>> @@ -3244,7 +3246,6 @@ static void igb_clean_tx_ring(struct igb_ring *tx_ring)
>> buffer_info = &tx_ring->tx_buffer_info[i];
>> igb_unmap_and_free_tx_resource(tx_ring, buffer_info);
>> }
>> - netdev_tx_reset_queue(txring_txq(tx_ring));
>>
> Why is it necessary to remove this? If rings are being freed with
> going through completion path then we would need this reset. Is this
> being done elsewhere maybe?
>
Alex moved this into the igb_configure_tx_ring() iirc to catch an ethtool
test case. He assured me when I reviewed the patch that igb_configure_tx_ring()
would always be called before netif_carrier_on() so I think(?) that should
be OK.
The above removed case was called after netif_carrier_off() anyways.
> Tom
> --
> 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
^ permalink raw reply
* Re: e1000e tx queue timeout in 3.3.0 (bisected to BQL support for e1000e)
From: Ben Greear @ 2012-04-20 21:24 UTC (permalink / raw)
To: Tom Herbert; +Cc: John Fastabend, netdev, e1000-devel list
In-Reply-To: <CA+mtBx-K6-O3ea34kmn4LXtx73FqCHF16w7pBSq1fHNN+-DxCA@mail.gmail.com>
On 04/20/2012 02:21 PM, Tom Herbert wrote:
> Thanks John for pointers to those. Ben, are you running a kernel with
> these patches?
I don't think so. I'll add them and re-test.
Thanks,
Ben
>
> Tom
>
>>
>> Tom, did you see these two patches? Maybe this is resolved by
>> the second patch.
>>
>> We needed these to fixup ixgbe and igb (i didn't test e1000e)
>> looks like we might want to push these at stable. I don't
>> believe they are in 3.3.
>>
>> commit b37c0fbe3f6dfba1f8ad2aed47fb40578a254635
>> Author: Alexander Duyck<alexander.h.duyck@intel.com>
>> Date: Tue Feb 7 02:29:06 2012 +0000
>>
>> net: Add memory barriers to prevent possible race in byte queue limits
>>
>> This change adds a memory barrier to the byte queue limit code to address a
>> possible race as has been seen in the past with the
>> netif_stop_queue/netif_wake_queue logic.
>>
>> Signed-off-by: Alexander Duyck<alexander.h.duyck@intel.com>
>> Tested-by: Stephen Ko<stephen.s.ko@intel.com>
>> Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>>
>>
>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=b37c0fbe3f6dfba1f8ad2aed47fb40578a254635
>>
>>
>> commit 5c4903549c05bbb373479e0ce2992573c120654a
>> Author: Alexander Duyck<alexander.h.duyck@intel.com>
>> Date: Tue Feb 7 02:29:01 2012 +0000
>>
>> net: Fix issue with netdev_tx_reset_queue not resetting queue from XOFF state
>>
>> We are seeing dev_watchdog hangs on several drivers. I suspect this is due
>> to the __QUEUE_STATE_STACK_XOFF bit being set prior to a reset for link
>> change, and then not being cleared by netdev_tx_reset_queue. This change
>> corrects that.
>>
>> In addition we were seeing dev_watchdog hangs on igb after running the
>> ethtool tests. We found this to be due to the fact that the ethtool test
>> runs the same logic as ndo_start_xmit, but we were never clearing the XOFF
>> flag since the loopback test in ethtool does not do byte queue accounting.
>>
>> Signed-off-by: Alexander Duyck<alexander.h.duyck@intel.com>
>> Tested-by: Stephen Ko<stephen.s.ko@intel.com>
>> Signed-off-by: Jeff Kirsher<jeffrey.t.kirsher@intel.com>
>>
>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5c4903549c05bbb373479e0ce2992573c120654a
>>
>>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
------------------------------------------------------------------------------
For Developers, A Lot Can Happen In A Second.
Boundary is the first to Know...and Tell You.
Monitor Your Applications in Ultra-Fine Resolution. Try it FREE!
http://p.sf.net/sfu/Boundary-d2dvs2
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply
* Re: e1000e tx queue timeout in 3.3.0 (bisected to BQL support for e1000e)
From: Tom Herbert @ 2012-04-20 21:21 UTC (permalink / raw)
To: John Fastabend; +Cc: Ben Greear, netdev, e1000-devel list, Eric Dumazet
In-Reply-To: <4F91BC82.2000804@intel.com>
Thanks John for pointers to those. Ben, are you running a kernel with
these patches?
Tom
>
> Tom, did you see these two patches? Maybe this is resolved by
> the second patch.
>
> We needed these to fixup ixgbe and igb (i didn't test e1000e)
> looks like we might want to push these at stable. I don't
> believe they are in 3.3.
>
> commit b37c0fbe3f6dfba1f8ad2aed47fb40578a254635
> Author: Alexander Duyck <alexander.h.duyck@intel.com>
> Date: Tue Feb 7 02:29:06 2012 +0000
>
> net: Add memory barriers to prevent possible race in byte queue limits
>
> This change adds a memory barrier to the byte queue limit code to address a
> possible race as has been seen in the past with the
> netif_stop_queue/netif_wake_queue logic.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Tested-by: Stephen Ko <stephen.s.ko@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>
>
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=b37c0fbe3f6dfba1f8ad2aed47fb40578a254635
>
>
> commit 5c4903549c05bbb373479e0ce2992573c120654a
> Author: Alexander Duyck <alexander.h.duyck@intel.com>
> Date: Tue Feb 7 02:29:01 2012 +0000
>
> net: Fix issue with netdev_tx_reset_queue not resetting queue from XOFF state
>
> We are seeing dev_watchdog hangs on several drivers. I suspect this is due
> to the __QUEUE_STATE_STACK_XOFF bit being set prior to a reset for link
> change, and then not being cleared by netdev_tx_reset_queue. This change
> corrects that.
>
> In addition we were seeing dev_watchdog hangs on igb after running the
> ethtool tests. We found this to be due to the fact that the ethtool test
> runs the same logic as ndo_start_xmit, but we were never clearing the XOFF
> flag since the loopback test in ethtool does not do byte queue accounting.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Tested-by: Stephen Ko <stephen.s.ko@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=5c4903549c05bbb373479e0ce2992573c120654a
>
>
^ permalink raw reply
* Re: [net-next 04/14] net: Fix issue with netdev_tx_reset_queue not resetting queue from XOFF state
From: Tom Herbert @ 2012-04-20 21:19 UTC (permalink / raw)
To: Jeff Kirsher; +Cc: davem, Alexander Duyck, netdev, gospo, sassmann
In-Reply-To: <1331611432-30109-5-git-send-email-jeffrey.t.kirsher@intel.com>
Hi Jeff,
> @@ -3244,7 +3246,6 @@ static void igb_clean_tx_ring(struct igb_ring *tx_ring)
> buffer_info = &tx_ring->tx_buffer_info[i];
> igb_unmap_and_free_tx_resource(tx_ring, buffer_info);
> }
> - netdev_tx_reset_queue(txring_txq(tx_ring));
>
Why is it necessary to remove this? If rings are being freed with
going through completion path then we would need this reset. Is this
being done elsewhere maybe?
Tom
^ permalink raw reply
* [PATCH net-next 16/16] tipc: Ensure network address change doesn't impact configuration service
From: Paul Gortmaker @ 2012-04-20 21:05 UTC (permalink / raw)
To: davem; +Cc: netdev, allan.stephens, ying.xue
In-Reply-To: <1334955924-907-1-git-send-email-paul.gortmaker@windriver.com>
From: Allan Stephens <allan.stephens@windriver.com>
Enhances command validation done by TIPC's configuration service so
that it works properly even if the node's network address is changed in
mid-operation. The default node address of <0.0.0> is now recognized as an
alias for "this node" even after a new network address has been assigned.
Signed-off-by: Allan Stephens <allan.stephens@windriver.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
net/tipc/config.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/tipc/config.c b/net/tipc/config.c
index f76d3b1..f5458ed 100644
--- a/net/tipc/config.c
+++ b/net/tipc/config.c
@@ -290,7 +290,7 @@ struct sk_buff *tipc_cfg_do_cmd(u32 orig_node, u16 cmd, const void *request_area
/* Check command authorization */
- if (likely(orig_node == tipc_own_addr)) {
+ if (likely(in_own_node(orig_node))) {
/* command is permitted */
} else if (cmd >= 0x8000) {
rep_tlv_buf = tipc_cfg_reply_error_string(TIPC_CFG_NOT_SUPPORTED
--
1.7.9.3
^ permalink raw reply related
* [PATCH net-next 15/16] tipc: Ensure network address change doesn't impact rejected message
From: Paul Gortmaker @ 2012-04-20 21:05 UTC (permalink / raw)
To: davem; +Cc: netdev, allan.stephens, ying.xue
In-Reply-To: <1334955924-907-1-git-send-email-paul.gortmaker@windriver.com>
From: Allan Stephens <allan.stephens@windriver.com>
Revises handling of a rejected message to ensure that a locally
originated message is returned properly even if the node's network
address is changed in mid-operation. The routine now treats the
default node address of <0.0.0> as an alias for "this node" when
determining where to send a returned message.
Signed-off-by: Allan Stephens <allan.stephens@windriver.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
net/tipc/port.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/tipc/port.c b/net/tipc/port.c
index c50819b..0f40b10 100644
--- a/net/tipc/port.c
+++ b/net/tipc/port.c
@@ -444,7 +444,7 @@ int tipc_reject_msg(struct sk_buff *buf, u32 err)
/* send returned message & dispose of rejected message */
src_node = msg_prevnode(msg);
- if (src_node == tipc_own_addr)
+ if (in_own_node(src_node))
tipc_port_recv_msg(rbuf);
else
tipc_link_send(rbuf, src_node, msg_link_selector(rmsg));
--
1.7.9.3
^ permalink raw reply related
* [PATCH net-next 14/16] tipc: handle <0.0.0> as an alias for this node on outgoing msgs
From: Paul Gortmaker @ 2012-04-20 21:05 UTC (permalink / raw)
To: davem; +Cc: netdev, allan.stephens, ying.xue
In-Reply-To: <1334955924-907-1-git-send-email-paul.gortmaker@windriver.com>
From: Allan Stephens <allan.stephens@windriver.com>
Revises handling of send routines for payload messages to ensure that
they are processed properly even if the node's network address is
changed in mid-operation. The routines now treat the default node
address of <0.0.0> as an alias for "this node" when determining where
to send an outgoing message.
Signed-off-by: Allan Stephens <allan.stephens@windriver.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
net/tipc/port.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/tipc/port.c b/net/tipc/port.c
index dc7f916..c50819b 100644
--- a/net/tipc/port.c
+++ b/net/tipc/port.c
@@ -1217,7 +1217,7 @@ int tipc_send(u32 ref, unsigned int num_sect, struct iovec const *msg_sect,
p_ptr->congested = 1;
if (!tipc_port_congested(p_ptr)) {
destnode = port_peernode(p_ptr);
- if (likely(destnode != tipc_own_addr))
+ if (likely(!in_own_node(destnode)))
res = tipc_link_send_sections_fast(p_ptr, msg_sect, num_sect,
total_len, destnode);
else
@@ -1267,7 +1267,7 @@ int tipc_send2name(u32 ref, struct tipc_name const *name, unsigned int domain,
msg_set_destport(msg, destport);
if (likely(destport || destnode)) {
- if (likely(destnode == tipc_own_addr))
+ if (likely(in_own_node(destnode)))
res = tipc_port_recv_sections(p_ptr, num_sect,
msg_sect, total_len);
else if (tipc_own_addr)
@@ -1315,7 +1315,7 @@ int tipc_send2port(u32 ref, struct tipc_portid const *dest,
msg_set_destport(msg, dest->ref);
msg_set_hdr_sz(msg, BASIC_H_SIZE);
- if (dest->node == tipc_own_addr)
+ if (in_own_node(dest->node))
res = tipc_port_recv_sections(p_ptr, num_sect, msg_sect,
total_len);
else if (tipc_own_addr)
@@ -1362,7 +1362,7 @@ int tipc_send_buf2port(u32 ref, struct tipc_portid const *dest,
skb_push(buf, BASIC_H_SIZE);
skb_copy_to_linear_data(buf, msg, BASIC_H_SIZE);
- if (dest->node == tipc_own_addr)
+ if (in_own_node(dest->node))
res = tipc_port_recv_msg(buf);
else
res = tipc_send_buf_fast(buf, dest->node);
--
1.7.9.3
^ permalink raw reply related
* [PATCH net-next 13/16] tipc: properly handle off-node send requests with invalid addr
From: Paul Gortmaker @ 2012-04-20 21:05 UTC (permalink / raw)
To: davem; +Cc: netdev, allan.stephens, ying.xue
In-Reply-To: <1334955924-907-1-git-send-email-paul.gortmaker@windriver.com>
From: Allan Stephens <allan.stephens@windriver.com>
There are two send routines that might conceivably be asked by an
application to send a message off-node when the node is still using
the default network address. These now have an added check that
detects this and rejects the message gracefully.
Signed-off-by: Allan Stephens <allan.stephens@windriver.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
net/tipc/port.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/net/tipc/port.c b/net/tipc/port.c
index 616c72f..dc7f916 100644
--- a/net/tipc/port.c
+++ b/net/tipc/port.c
@@ -1270,10 +1270,14 @@ int tipc_send2name(u32 ref, struct tipc_name const *name, unsigned int domain,
if (likely(destnode == tipc_own_addr))
res = tipc_port_recv_sections(p_ptr, num_sect,
msg_sect, total_len);
- else
+ else if (tipc_own_addr)
res = tipc_link_send_sections_fast(p_ptr, msg_sect,
num_sect, total_len,
destnode);
+ else
+ res = tipc_port_reject_sections(p_ptr, msg, msg_sect,
+ num_sect, total_len,
+ TIPC_ERR_NO_NODE);
if (likely(res != -ELINKCONG)) {
if (res > 0)
p_ptr->sent++;
@@ -1314,9 +1318,12 @@ int tipc_send2port(u32 ref, struct tipc_portid const *dest,
if (dest->node == tipc_own_addr)
res = tipc_port_recv_sections(p_ptr, num_sect, msg_sect,
total_len);
- else
+ else if (tipc_own_addr)
res = tipc_link_send_sections_fast(p_ptr, msg_sect, num_sect,
total_len, dest->node);
+ else
+ res = tipc_port_reject_sections(p_ptr, msg, msg_sect, num_sect,
+ total_len, TIPC_ERR_NO_NODE);
if (likely(res != -ELINKCONG)) {
if (res > 0)
p_ptr->sent++;
--
1.7.9.3
^ permalink raw reply related
* [PATCH net-next 12/16] tipc: take lock while updating node network address
From: Paul Gortmaker @ 2012-04-20 21:05 UTC (permalink / raw)
To: davem; +Cc: netdev, allan.stephens, ying.xue
In-Reply-To: <1334955924-907-1-git-send-email-paul.gortmaker@windriver.com>
From: Allan Stephens <allan.stephens@windriver.com>
The routine that changes the node's network address now takes TIPC's
network lock in write mode while the main address variable and associated
data structures are being changed; this is needed to ensure that the
link subsystem won't attempt to send a message off-node until the sending
port's message header template has been updated with the node's new
network address.
Signed-off-by: Allan Stephens <allan.stephens@windriver.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
net/tipc/net.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/tipc/net.c b/net/tipc/net.c
index d4531b0..5fab4ff 100644
--- a/net/tipc/net.c
+++ b/net/tipc/net.c
@@ -178,11 +178,12 @@ int tipc_net_start(u32 addr)
tipc_subscr_stop();
tipc_cfg_stop();
+ write_lock_bh(&tipc_net_lock);
tipc_own_addr = addr;
tipc_named_reinit();
tipc_port_reinit();
-
tipc_bclink_init();
+ write_unlock_bh(&tipc_net_lock);
tipc_k_signal((Handler)tipc_subscr_start, 0);
tipc_k_signal((Handler)tipc_cfg_init, 0);
--
1.7.9.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox