| Message ID | m1ipsob6f8.fsf@fess.ebiederm.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Jun 2, 2011 at 11:26 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > Changli Gao <xiaosuo@gmail.com> writes: > >> On Thu, Jun 2, 2011 at 9:03 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: >>> >>> -static struct sk_buff *vlan_check_reorder_header(struct sk_buff *skb) >>> +static struct sk_buff *vlan_reorder_header(struct sk_buff *skb) >>> { >>> - if (vlan_dev_info(skb->dev)->flags & VLAN_FLAG_REORDER_HDR) { >>> - if (skb_cow(skb, skb_headroom(skb)) < 0) >>> - skb = NULL; >>> - if (skb) { >>> - /* Lifted from Gleb's VLAN code... */ >>> - memmove(skb->data - ETH_HLEN, >>> - skb->data - VLAN_ETH_HLEN, 12); >>> - skb->mac_header += VLAN_HLEN; >>> - } >>> + if (skb_cow(skb, skb_headroom(skb)) < 0) >>> + skb = NULL; >>> + if (skb) { >> >> I think an else branch maybe more readable here. > > Probably most readable would be simply returning NULL immediately on > error. In this patch I just removed the if statement, and I would like > to avoid mixing different bug fixes in the same patch if possible. > OK. It is minor. >>> + /* Lifted from Gleb's VLAN code... */ >>> + memmove(skb->data - ETH_HLEN, >>> + skb->data - VLAN_ETH_HLEN, 12); >>> + skb->mac_header += VLAN_HLEN; >> >> skb->mac_len should be adjusted too. > > Given how vlan_untag is called at the moment it does appear > that the skb->mac_len = skb->network_header - skb->mac_header > in __netif_receive_skb that used to handle this for is no longer > doing this for us. > > My feel is that either we need to do all of the header resets and the > vlan_untagging together. So we either need this all together before or > after the another_round label: > > So the proper fix is probably something like this. > OK, it is right. Thanks.
Thu, Jun 02, 2011 at 05:26:51PM CEST, ebiederm@xmission.com wrote: >Changli Gao <xiaosuo@gmail.com> writes: > >> On Thu, Jun 2, 2011 at 9:03 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: >>> >>> -static struct sk_buff *vlan_check_reorder_header(struct sk_buff *skb) >>> +static struct sk_buff *vlan_reorder_header(struct sk_buff *skb) >>> { >>> - if (vlan_dev_info(skb->dev)->flags & VLAN_FLAG_REORDER_HDR) { >>> - if (skb_cow(skb, skb_headroom(skb)) < 0) >>> - skb = NULL; >>> - if (skb) { >>> - /* Lifted from Gleb's VLAN code... */ >>> - memmove(skb->data - ETH_HLEN, >>> - skb->data - VLAN_ETH_HLEN, 12); >>> - skb->mac_header += VLAN_HLEN; >>> - } >>> + if (skb_cow(skb, skb_headroom(skb)) < 0) >>> + skb = NULL; >>> + if (skb) { >> >> I think an else branch maybe more readable here. > >Probably most readable would be simply returning NULL immediately on >error. In this patch I just removed the if statement, and I would like >to avoid mixing different bug fixes in the same patch if possible. > >>> + /* Lifted from Gleb's VLAN code... */ >>> + memmove(skb->data - ETH_HLEN, >>> + skb->data - VLAN_ETH_HLEN, 12); >>> + skb->mac_header += VLAN_HLEN; >> >> skb->mac_len should be adjusted too. > >Given how vlan_untag is called at the moment it does appear >that the skb->mac_len = skb->network_header - skb->mac_header >in __netif_receive_skb that used to handle this for is no longer >doing this for us. > >My feel is that either we need to do all of the header resets and the >vlan_untagging together. So we either need this all together before or >after the another_round label: > >So the proper fix is probably something like this. > >diff --git a/net/core/dev.c b/net/core/dev.c >index bcb05cb..8fe50d4 100644 >--- a/net/core/dev.c >+++ b/net/core/dev.c >@@ -3102,9 +3102,6 @@ static int __netif_receive_skb(struct sk_buff *skb) > skb->skb_iif = skb->dev->ifindex; > orig_dev = skb->dev; > >- skb_reset_network_header(skb); >- skb_reset_transport_header(skb); >- skb->mac_len = skb->network_header - skb->mac_header; > > pt_prev = NULL; > >@@ -3119,6 +3116,9 @@ another_round: > if (unlikely(!skb)) > goto out; > } >+ skb_reset_network_header(skb); >+ skb_reset_transport_header(skb); >+ skb->mac_len = skb->network_header - skb->mac_header; > > #ifdef CONFIG_NET_CLS_ACT > if (skb->tc_verd & TC_NCLS) { This looks good to me. This does not need to be done before vlan_untag. -- 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/dev.c b/net/core/dev.c index bcb05cb..8fe50d4 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -3102,9 +3102,6 @@ static int __netif_receive_skb(struct sk_buff *skb) skb->skb_iif = skb->dev->ifindex; orig_dev = skb->dev; - skb_reset_network_header(skb); - skb_reset_transport_header(skb); - skb->mac_len = skb->network_header - skb->mac_header; pt_prev = NULL; @@ -3119,6 +3116,9 @@ another_round: if (unlikely(!skb)) goto out; } + skb_reset_network_header(skb); + skb_reset_transport_header(skb); + skb->mac_len = skb->network_header - skb->mac_header; #ifdef CONFIG_NET_CLS_ACT if (skb->tc_verd & TC_NCLS) {