* [RFC PATCH] net act_vlan: use correct len in skb_pull
@ 2019-02-13 19:51 Zahari Doychev
2019-02-14 9:16 ` Toshiaki Makita
0 siblings, 1 reply; 3+ messages in thread
From: Zahari Doychev @ 2019-02-13 19:51 UTC (permalink / raw)
To: netdev, bridge, makita.toshiaki, nikolay, roopa, jhs, jiri,
xiyou.wangcong
Cc: johannes, zahari.doychev
The bridge and VLAN code expects that skb->data points to the start of the
VLAN header instead of the next (network) header. Currently after
tcf_vlan_act() on ingress filter skb->data points to the next network
header. In this case the Linux bridge does not forward correctly double
tagged VLAN packets added using tc vlan action as the outer vlan tag from
the skb is inserted at the wrong offset after the vlan tag in the payload.
Making skb->data to point to the VLAN header in tcf_vlan_act() by using
ETH_HLEN in skb_pull_rcsum() fixes the problem.
The following commands were used for testing:
ip link add name br0 type bridge vlan_filtering 1
ip link set dev br0 up
ip link set dev net0 up
ip link set dev net0 master br0
ip link set dev net1 up
ip link set dev net1 master br0
bridge vlan add dev net0 vid 100 master
bridge vlan add dev br0 vid 100 self
bridge vlan add dev net1 vid 100 master
tc qdisc add dev net0 handle ffff: clsact
tc qdisc add dev net1 handle ffff: clsact
tc filter add dev net0 ingress pref 1 protocol all flower \
action vlan push id 10 pipe action vlan push id 100
tc filter add dev net0 egress pref 1 protocol 802.1q flower \
vlan_id 100 vlan_ethtype 802.1q cvlan_id 10 \
action vlan pop pipe action vlan pop
Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
---
net/sched/act_vlan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
index 93fdaf707313..308d7d89f925 100644
--- a/net/sched/act_vlan.c
+++ b/net/sched/act_vlan.c
@@ -86,7 +86,7 @@ static int tcf_vlan_act(struct sk_buff *skb, const struct tc_action *a,
out:
if (skb_at_tc_ingress(skb))
- skb_pull_rcsum(skb, skb->mac_len);
+ skb_pull_rcsum(skb, ETH_HLEN);
return action;
--
2.20.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [RFC PATCH] net act_vlan: use correct len in skb_pull
2019-02-13 19:51 [RFC PATCH] net act_vlan: use correct len in skb_pull Zahari Doychev
@ 2019-02-14 9:16 ` Toshiaki Makita
2019-02-14 11:54 ` Zahari Doychev
0 siblings, 1 reply; 3+ messages in thread
From: Toshiaki Makita @ 2019-02-14 9:16 UTC (permalink / raw)
To: Zahari Doychev, netdev, bridge, nikolay, roopa, jhs, jiri,
xiyou.wangcong
Cc: johannes
On 2019/02/14 4:51, Zahari Doychev wrote:
> The bridge and VLAN code expects that skb->data points to the start of the
> VLAN header instead of the next (network) header. Currently after
> tcf_vlan_act() on ingress filter skb->data points to the next network
> header. In this case the Linux bridge does not forward correctly double
> tagged VLAN packets added using tc vlan action as the outer vlan tag from
> the skb is inserted at the wrong offset after the vlan tag in the payload.
> Making skb->data to point to the VLAN header in tcf_vlan_act() by using
> ETH_HLEN in skb_pull_rcsum() fixes the problem.
>
> The following commands were used for testing:
>
> ip link add name br0 type bridge vlan_filtering 1
> ip link set dev br0 up
>
> ip link set dev net0 up
> ip link set dev net0 master br0
>
> ip link set dev net1 up
> ip link set dev net1 master br0
>
> bridge vlan add dev net0 vid 100 master
> bridge vlan add dev br0 vid 100 self
> bridge vlan add dev net1 vid 100 master
>
> tc qdisc add dev net0 handle ffff: clsact
> tc qdisc add dev net1 handle ffff: clsact
>
> tc filter add dev net0 ingress pref 1 protocol all flower \
> action vlan push id 10 pipe action vlan push id 100
>
> tc filter add dev net0 egress pref 1 protocol 802.1q flower \
> vlan_id 100 vlan_ethtype 802.1q cvlan_id 10 \
> action vlan pop pipe action vlan pop
>
> Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
> ---
> net/sched/act_vlan.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
> index 93fdaf707313..308d7d89f925 100644
> --- a/net/sched/act_vlan.c
> +++ b/net/sched/act_vlan.c
> @@ -86,7 +86,7 @@ static int tcf_vlan_act(struct sk_buff *skb, const struct tc_action *a,
>
> out:
> if (skb_at_tc_ingress(skb))
> - skb_pull_rcsum(skb, skb->mac_len);
> + skb_pull_rcsum(skb, ETH_HLEN);
As I said before, it would be safer to remember mac_len and use it later.
__u16 mac_len = skb->mac_len;
...
err = skb_vlan_push(...)
...
if (skb_at_tc_ingress(skb))
skb_pull_rcsum(skb, mac_len);
--
Toshiaki Makita
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFC PATCH] net act_vlan: use correct len in skb_pull
2019-02-14 9:16 ` Toshiaki Makita
@ 2019-02-14 11:54 ` Zahari Doychev
0 siblings, 0 replies; 3+ messages in thread
From: Zahari Doychev @ 2019-02-14 11:54 UTC (permalink / raw)
To: Toshiaki Makita
Cc: netdev, bridge, nikolay, roopa, jhs, jiri, xiyou.wangcong,
johannes
On Thu, Feb 14, 2019 at 06:16:12PM +0900, Toshiaki Makita wrote:
> On 2019/02/14 4:51, Zahari Doychev wrote:
> > The bridge and VLAN code expects that skb->data points to the start of the
> > VLAN header instead of the next (network) header. Currently after
> > tcf_vlan_act() on ingress filter skb->data points to the next network
> > header. In this case the Linux bridge does not forward correctly double
> > tagged VLAN packets added using tc vlan action as the outer vlan tag from
> > the skb is inserted at the wrong offset after the vlan tag in the payload.
> > Making skb->data to point to the VLAN header in tcf_vlan_act() by using
> > ETH_HLEN in skb_pull_rcsum() fixes the problem.
> >
> > The following commands were used for testing:
> >
> > ip link add name br0 type bridge vlan_filtering 1
> > ip link set dev br0 up
> >
> > ip link set dev net0 up
> > ip link set dev net0 master br0
> >
> > ip link set dev net1 up
> > ip link set dev net1 master br0
> >
> > bridge vlan add dev net0 vid 100 master
> > bridge vlan add dev br0 vid 100 self
> > bridge vlan add dev net1 vid 100 master
> >
> > tc qdisc add dev net0 handle ffff: clsact
> > tc qdisc add dev net1 handle ffff: clsact
> >
> > tc filter add dev net0 ingress pref 1 protocol all flower \
> > action vlan push id 10 pipe action vlan push id 100
> >
> > tc filter add dev net0 egress pref 1 protocol 802.1q flower \
> > vlan_id 100 vlan_ethtype 802.1q cvlan_id 10 \
> > action vlan pop pipe action vlan pop
> >
> > Signed-off-by: Zahari Doychev <zahari.doychev@linux.com>
> > ---
> > net/sched/act_vlan.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/sched/act_vlan.c b/net/sched/act_vlan.c
> > index 93fdaf707313..308d7d89f925 100644
> > --- a/net/sched/act_vlan.c
> > +++ b/net/sched/act_vlan.c
> > @@ -86,7 +86,7 @@ static int tcf_vlan_act(struct sk_buff *skb, const struct tc_action *a,
> >
> > out:
> > if (skb_at_tc_ingress(skb))
> > - skb_pull_rcsum(skb, skb->mac_len);
> > + skb_pull_rcsum(skb, ETH_HLEN);
>
> As I said before, it would be safer to remember mac_len and use it later.
>
> __u16 mac_len = skb->mac_len;
> ...
> err = skb_vlan_push(...)
> ...
> if (skb_at_tc_ingress(skb))
> skb_pull_rcsum(skb, mac_len);
>
sorry, I misunderstood it. I will send an update.
Zahari
>
> --
> Toshiaki Makita
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-02-14 11:19 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-13 19:51 [RFC PATCH] net act_vlan: use correct len in skb_pull Zahari Doychev
2019-02-14 9:16 ` Toshiaki Makita
2019-02-14 11:54 ` Zahari Doychev
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).