* [PATCH] act_mirred: do not drop packets when fails to mirror it
@ 2012-08-15 9:37 Jason Wang
2012-08-15 12:35 ` Jamal Hadi Salim
0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2012-08-15 9:37 UTC (permalink / raw)
To: netdev, jhs, linux-kernel, davem
We drop packet unconditionally when we fail to mirror it. This is not intended
in some cases. Consdier for kvm guest, we may mirror the traffic of the bridge
to a tap device used by a VM. When kernel fails to mirror the packet in
conditions such as when qemu crashes or stop polling the tap, it's hard for the
management software to detect such condition and clean the the mirroring
before. This would lead all packets to the bridge to be dropped and break the
netowrk of other virtual machines.
To solve the issue, the patch does not drop packets when kernel fails to mirror
it, and only drop the redirected packets.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
net/sched/act_mirred.c | 9 +++------
1 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index fe81cc1..3682951 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -198,15 +198,12 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
err = dev_queue_xmit(skb2);
out:
- if (err) {
+ if (err)
m->tcf_qstats.overlimits++;
- /* should we be asking for packet to be dropped?
- * may make sense for redirect case only
- */
+ if (err && m->tcf_action == TC_ACT_STOLEN)
retval = TC_ACT_SHOT;
- } else {
+ else
retval = m->tcf_action;
- }
spin_unlock(&m->tcf_lock);
return retval;
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] act_mirred: do not drop packets when fails to mirror it
2012-08-15 9:37 [PATCH] act_mirred: do not drop packets when fails to mirror it Jason Wang
@ 2012-08-15 12:35 ` Jamal Hadi Salim
2012-08-15 13:42 ` Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: Jamal Hadi Salim @ 2012-08-15 12:35 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, davem
On Wed, 2012-08-15 at 17:37 +0800, Jason Wang wrote:
> We drop packet unconditionally when we fail to mirror it. This is not intended
> in some cases.
Hi Jason,
Did you actually notice the behavior you described or were you going by
the XXX comment I had in the code?
cheers,
jamal
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] act_mirred: do not drop packets when fails to mirror it
2012-08-15 12:35 ` Jamal Hadi Salim
@ 2012-08-15 13:42 ` Jason Wang
2012-08-15 15:10 ` Jamal Hadi Salim
0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2012-08-15 13:42 UTC (permalink / raw)
To: jhs; +Cc: netdev, linux-kernel, davem
On 08/15/2012 08:35 PM, Jamal Hadi Salim wrote:
> On Wed, 2012-08-15 at 17:37 +0800, Jason Wang wrote:
>> We drop packet unconditionally when we fail to mirror it. This is not intended
>> in some cases.
> Hi Jason,
> Did you actually notice the behavior you described or were you going by
> the XXX comment I had in the code?
>
> cheers,
> jamal
Hi Jamal:
I met it actually through the following steps:
- start a kvm guest with tap and make it to be an interface of the bridge
- mirror the ingress traffic of the bridge to the tap
- terminate the qemu process, the tap device is then removed
- all packet goes to bridge would be dropped, so the network of guests
in the same bridge would be broken
It's hard for the management to forcast the termination of the porcess
and clean the mirroring before. The realistic way is to remove the
mirroring after the termination of the process. So, if we drop the
packets when the mirred device (tap) is removed, in the gap between the
qemu termitnaion and disabling mirroring, the bridge ( and other VMs
using the it) would not recevie any packet.
>
> --
> 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 [flat|nested] 5+ messages in thread
* Re: [PATCH] act_mirred: do not drop packets when fails to mirror it
2012-08-15 13:42 ` Jason Wang
@ 2012-08-15 15:10 ` Jamal Hadi Salim
2012-08-16 6:40 ` Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: Jamal Hadi Salim @ 2012-08-15 15:10 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, davem
On Wed, 2012-08-15 at 21:42 +0800, Jason Wang wrote:
>
> I met it actually through the following steps:
>
> - start a kvm guest with tap and make it to be an interface of the bridge
> - mirror the ingress traffic of the bridge to the tap
> - terminate the qemu process, the tap device is then removed
> - all packet goes to bridge would be dropped, so the network of guests
> in the same bridge would be broken
>
Makes sense.
Can you please leave the err check braces i.e
if (err) {
m->tcf_qstats.overlimits++;
if (m->tcfm_eaction != TCA_EGRESS_MIRROR)
retval = TC_ACT_SHOT;
else
retval = m->tcf_action;
} else {
retval = m->tcf_action;
}
Or at least dont use TC_ACT_STOLEN.
cheers,
jamal
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] act_mirred: do not drop packets when fails to mirror it
2012-08-15 15:10 ` Jamal Hadi Salim
@ 2012-08-16 6:40 ` Jason Wang
0 siblings, 0 replies; 5+ messages in thread
From: Jason Wang @ 2012-08-16 6:40 UTC (permalink / raw)
To: jhs; +Cc: netdev, linux-kernel, davem
On 08/15/2012 11:10 PM, Jamal Hadi Salim wrote:
> On Wed, 2012-08-15 at 21:42 +0800, Jason Wang wrote:
>
>> I met it actually through the following steps:
>>
>> - start a kvm guest with tap and make it to be an interface of the bridge
>> - mirror the ingress traffic of the bridge to the tap
>> - terminate the qemu process, the tap device is then removed
>> - all packet goes to bridge would be dropped, so the network of guests
>> in the same bridge would be broken
>>
> Makes sense.
> Can you please leave the err check braces i.e
> if (err) {
> m->tcf_qstats.overlimits++;
> if (m->tcfm_eaction != TCA_EGRESS_MIRROR)
> retval = TC_ACT_SHOT;
> else
> retval = m->tcf_action;
> } else {
> retval = m->tcf_action;
> }
>
> Or at least dont use TC_ACT_STOLEN.
>
> cheers,
> jamal
>
Sure, will post v2. Thanks.
> --
> 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 [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-08-16 6:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-15 9:37 [PATCH] act_mirred: do not drop packets when fails to mirror it Jason Wang
2012-08-15 12:35 ` Jamal Hadi Salim
2012-08-15 13:42 ` Jason Wang
2012-08-15 15:10 ` Jamal Hadi Salim
2012-08-16 6:40 ` Jason Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).