* [PATCH net] bridge: switchdev: Clear forward mark when transmitting packet
@ 2017-09-01 9:22 Ido Schimmel
2017-09-01 11:45 ` Nikolay Aleksandrov
2017-09-01 17:12 ` David Miller
0 siblings, 2 replies; 5+ messages in thread
From: Ido Schimmel @ 2017-09-01 9:22 UTC (permalink / raw)
To: netdev; +Cc: davem, stephen, nikolay, jiri, yotamg, mlxsw, bridge, Ido Schimmel
Commit 6bc506b4fb06 ("bridge: switchdev: Add forward mark support for
stacked devices") added the 'offload_fwd_mark' bit to the skb in order
to allow drivers to indicate to the bridge driver that they already
forwarded the packet in L2.
In case the bit is set, before transmitting the packet from each port,
the port's mark is compared with the mark stored in the skb's control
block. If both marks are equal, we know the packet arrived from a switch
device that already forwarded the packet and it's not re-transmitted.
However, if the packet is transmitted from the bridge device itself
(e.g., br0), we should clear the 'offload_fwd_mark' bit as the mark
stored in the skb's control block isn't valid.
This scenario can happen in rare cases where a packet was trapped during
L3 forwarding and forwarded by the kernel to a bridge device.
Fixes: 6bc506b4fb06 ("bridge: switchdev: Add forward mark support for stacked devices")
Signed-off-by: Ido Schimmel <idosch@mellanox.com>
Reported-by: Yotam Gigi <yotamg@mellanox.com>
Tested-by: Yotam Gigi <yotamg@mellanox.com>
Reviewed-by: Jiri Pirko <jiri@mellanox.com>
---
net/bridge/br_device.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
index 861ae2a165f4..5a7be3bddfa9 100644
--- a/net/bridge/br_device.c
+++ b/net/bridge/br_device.c
@@ -53,6 +53,9 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
brstats->tx_bytes += skb->len;
u64_stats_update_end(&brstats->syncp);
+#ifdef CONFIG_NET_SWITCHDEV
+ skb->offload_fwd_mark = 0;
+#endif
BR_INPUT_SKB_CB(skb)->brdev = dev;
skb_reset_mac_header(skb);
--
2.13.5
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH net] bridge: switchdev: Clear forward mark when transmitting packet
2017-09-01 9:22 [PATCH net] bridge: switchdev: Clear forward mark when transmitting packet Ido Schimmel
@ 2017-09-01 11:45 ` Nikolay Aleksandrov
2017-09-01 12:01 ` Jiri Pirko
2017-09-01 17:12 ` David Miller
1 sibling, 1 reply; 5+ messages in thread
From: Nikolay Aleksandrov @ 2017-09-01 11:45 UTC (permalink / raw)
To: Ido Schimmel, netdev; +Cc: davem, stephen, jiri, yotamg, mlxsw, bridge
On 01/09/17 12:22, Ido Schimmel wrote:
> Commit 6bc506b4fb06 ("bridge: switchdev: Add forward mark support for
> stacked devices") added the 'offload_fwd_mark' bit to the skb in order
> to allow drivers to indicate to the bridge driver that they already
> forwarded the packet in L2.
>
> In case the bit is set, before transmitting the packet from each port,
> the port's mark is compared with the mark stored in the skb's control
> block. If both marks are equal, we know the packet arrived from a switch
> device that already forwarded the packet and it's not re-transmitted.
>
> However, if the packet is transmitted from the bridge device itself
> (e.g., br0), we should clear the 'offload_fwd_mark' bit as the mark
> stored in the skb's control block isn't valid.
>
> This scenario can happen in rare cases where a packet was trapped during
> L3 forwarding and forwarded by the kernel to a bridge device.
>
> Fixes: 6bc506b4fb06 ("bridge: switchdev: Add forward mark support for stacked devices")
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Reported-by: Yotam Gigi <yotamg@mellanox.com>
> Tested-by: Yotam Gigi <yotamg@mellanox.com>
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
> ---
> net/bridge/br_device.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index 861ae2a165f4..5a7be3bddfa9 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -53,6 +53,9 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
> brstats->tx_bytes += skb->len;
> u64_stats_update_end(&brstats->syncp);
>
> +#ifdef CONFIG_NET_SWITCHDEV
> + skb->offload_fwd_mark = 0;
> +#endif
> BR_INPUT_SKB_CB(skb)->brdev = dev;
>
> skb_reset_mac_header(skb);
>
Good catch, just one minor nit since there is already an ifdef
switchdev/else in br_private.h, why not make this a helper and avoid the
ifdef/endif in here ? Currently there is no ifdef switchdev anywhere else.
Thanks,
Nik
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] bridge: switchdev: Clear forward mark when transmitting packet
2017-09-01 11:45 ` Nikolay Aleksandrov
@ 2017-09-01 12:01 ` Jiri Pirko
2017-09-01 12:03 ` Nikolay Aleksandrov
0 siblings, 1 reply; 5+ messages in thread
From: Jiri Pirko @ 2017-09-01 12:01 UTC (permalink / raw)
To: Nikolay Aleksandrov
Cc: Ido Schimmel, netdev, davem, stephen, jiri, yotamg, mlxsw, bridge
Fri, Sep 01, 2017 at 01:45:15PM CEST, nikolay@cumulusnetworks.com wrote:
>On 01/09/17 12:22, Ido Schimmel wrote:
>> Commit 6bc506b4fb06 ("bridge: switchdev: Add forward mark support for
>> stacked devices") added the 'offload_fwd_mark' bit to the skb in order
>> to allow drivers to indicate to the bridge driver that they already
>> forwarded the packet in L2.
>>
>> In case the bit is set, before transmitting the packet from each port,
>> the port's mark is compared with the mark stored in the skb's control
>> block. If both marks are equal, we know the packet arrived from a switch
>> device that already forwarded the packet and it's not re-transmitted.
>>
>> However, if the packet is transmitted from the bridge device itself
>> (e.g., br0), we should clear the 'offload_fwd_mark' bit as the mark
>> stored in the skb's control block isn't valid.
>>
>> This scenario can happen in rare cases where a packet was trapped during
>> L3 forwarding and forwarded by the kernel to a bridge device.
>>
>> Fixes: 6bc506b4fb06 ("bridge: switchdev: Add forward mark support for stacked devices")
>> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
>> Reported-by: Yotam Gigi <yotamg@mellanox.com>
>> Tested-by: Yotam Gigi <yotamg@mellanox.com>
>> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>> net/bridge/br_device.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
>> index 861ae2a165f4..5a7be3bddfa9 100644
>> --- a/net/bridge/br_device.c
>> +++ b/net/bridge/br_device.c
>> @@ -53,6 +53,9 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>> brstats->tx_bytes += skb->len;
>> u64_stats_update_end(&brstats->syncp);
>>
>> +#ifdef CONFIG_NET_SWITCHDEV
>> + skb->offload_fwd_mark = 0;
>> +#endif
>> BR_INPUT_SKB_CB(skb)->brdev = dev;
>>
>> skb_reset_mac_header(skb);
>>
>
>Good catch, just one minor nit since there is already an ifdef
>switchdev/else in br_private.h, why not make this a helper and avoid the
>ifdef/endif in here ? Currently there is no ifdef switchdev anywhere else.
I think it would be better to convert this to a helper in -net-next and
take the patch as it is for -net
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] bridge: switchdev: Clear forward mark when transmitting packet
2017-09-01 12:01 ` Jiri Pirko
@ 2017-09-01 12:03 ` Nikolay Aleksandrov
0 siblings, 0 replies; 5+ messages in thread
From: Nikolay Aleksandrov @ 2017-09-01 12:03 UTC (permalink / raw)
To: Jiri Pirko; +Cc: mlxsw, yotamg, netdev, bridge, Ido Schimmel, jiri, davem
On 01/09/17 15:01, Jiri Pirko wrote:
> Fri, Sep 01, 2017 at 01:45:15PM CEST, nikolay@cumulusnetworks.com wrote:
>> On 01/09/17 12:22, Ido Schimmel wrote:
>>> Commit 6bc506b4fb06 ("bridge: switchdev: Add forward mark support for
>>> stacked devices") added the 'offload_fwd_mark' bit to the skb in order
>>> to allow drivers to indicate to the bridge driver that they already
>>> forwarded the packet in L2.
>>>
>>> In case the bit is set, before transmitting the packet from each port,
>>> the port's mark is compared with the mark stored in the skb's control
>>> block. If both marks are equal, we know the packet arrived from a switch
>>> device that already forwarded the packet and it's not re-transmitted.
>>>
>>> However, if the packet is transmitted from the bridge device itself
>>> (e.g., br0), we should clear the 'offload_fwd_mark' bit as the mark
>>> stored in the skb's control block isn't valid.
>>>
>>> This scenario can happen in rare cases where a packet was trapped during
>>> L3 forwarding and forwarded by the kernel to a bridge device.
>>>
>>> Fixes: 6bc506b4fb06 ("bridge: switchdev: Add forward mark support for stacked devices")
>>> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
>>> Reported-by: Yotam Gigi <yotamg@mellanox.com>
>>> Tested-by: Yotam Gigi <yotamg@mellanox.com>
>>> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
>>> ---
>>> net/bridge/br_device.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
>>> index 861ae2a165f4..5a7be3bddfa9 100644
>>> --- a/net/bridge/br_device.c
>>> +++ b/net/bridge/br_device.c
>>> @@ -53,6 +53,9 @@ netdev_tx_t br_dev_xmit(struct sk_buff *skb, struct net_device *dev)
>>> brstats->tx_bytes += skb->len;
>>> u64_stats_update_end(&brstats->syncp);
>>>
>>> +#ifdef CONFIG_NET_SWITCHDEV
>>> + skb->offload_fwd_mark = 0;
>>> +#endif
>>> BR_INPUT_SKB_CB(skb)->brdev = dev;
>>>
>>> skb_reset_mac_header(skb);
>>>
>>
>> Good catch, just one minor nit since there is already an ifdef
>> switchdev/else in br_private.h, why not make this a helper and avoid the
>> ifdef/endif in here ? Currently there is no ifdef switchdev anywhere else.
>
> I think it would be better to convert this to a helper in -net-next and
> take the patch as it is for -net
>
Either way is fine I guess, it's just more work for something as simple. :-)
Whichever way you choose,
Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH net] bridge: switchdev: Clear forward mark when transmitting packet
2017-09-01 9:22 [PATCH net] bridge: switchdev: Clear forward mark when transmitting packet Ido Schimmel
2017-09-01 11:45 ` Nikolay Aleksandrov
@ 2017-09-01 17:12 ` David Miller
1 sibling, 0 replies; 5+ messages in thread
From: David Miller @ 2017-09-01 17:12 UTC (permalink / raw)
To: idosch; +Cc: mlxsw, yotamg, nikolay, netdev, bridge, jiri
From: Ido Schimmel <idosch@mellanox.com>
Date: Fri, 1 Sep 2017 12:22:25 +0300
> Commit 6bc506b4fb06 ("bridge: switchdev: Add forward mark support for
> stacked devices") added the 'offload_fwd_mark' bit to the skb in order
> to allow drivers to indicate to the bridge driver that they already
> forwarded the packet in L2.
>
> In case the bit is set, before transmitting the packet from each port,
> the port's mark is compared with the mark stored in the skb's control
> block. If both marks are equal, we know the packet arrived from a switch
> device that already forwarded the packet and it's not re-transmitted.
>
> However, if the packet is transmitted from the bridge device itself
> (e.g., br0), we should clear the 'offload_fwd_mark' bit as the mark
> stored in the skb's control block isn't valid.
>
> This scenario can happen in rare cases where a packet was trapped during
> L3 forwarding and forwarded by the kernel to a bridge device.
>
> Fixes: 6bc506b4fb06 ("bridge: switchdev: Add forward mark support for stacked devices")
> Signed-off-by: Ido Schimmel <idosch@mellanox.com>
> Reported-by: Yotam Gigi <yotamg@mellanox.com>
> Tested-by: Yotam Gigi <yotamg@mellanox.com>
> Reviewed-by: Jiri Pirko <jiri@mellanox.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-09-01 17:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-01 9:22 [PATCH net] bridge: switchdev: Clear forward mark when transmitting packet Ido Schimmel
2017-09-01 11:45 ` Nikolay Aleksandrov
2017-09-01 12:01 ` Jiri Pirko
2017-09-01 12:03 ` Nikolay Aleksandrov
2017-09-01 17:12 ` David Miller
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).