| Message ID | m11uzcidvq.fsf_-_@fess.ebiederm.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Thu, Jun 02, 2011 at 03:03:53PM CEST, ebiederm@xmission.com wrote: > >Testing of VLAN_FLAG_REORDER_HDR does not belong in vlan_untag >but rather in vlan_do_receive. Otherwise the vlan header >will not be properly put on the packet in the case of >vlan header accelleration. > >As we remove the check from vlan_check_reorder_header >rename it vlan_reorder_header to keep the naming clean. > >Fix up the skb->pkt_type early so we don't look at the packet >after adding the vlan tag, which guarantees we don't goof >and look at the wrong field. > >Use a simple if statement instead of a complicated switch >statement to decided that we need to increment rx_stats >for a multicast packet. > >Hopefully at somepoint we will just declare the case where >VLAN_FLAG_REORDER_HDR is cleared as unsupported and remove >the code. Until then this keeps it working correctly. > >Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> Looking good to me. Thanks Eric. Reviewed-by: Jiri Pirko <jpirko@redhat.com> -- 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
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. > + /* 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. > } > return skb; > }
Doesnt __vlan_put_tag()/vlan_insert_tag() depend on skb->data pointing to ethernet header. Is'nt the skb->data pointing past ethernet header in netif_receive_skb(). Am I missing anything? Regards, Padmanabh On Thu, Jun 2, 2011 at 6:33 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > > Testing of VLAN_FLAG_REORDER_HDR does not belong in vlan_untag > but rather in vlan_do_receive. Otherwise the vlan header > will not be properly put on the packet in the case of > vlan header accelleration. > > As we remove the check from vlan_check_reorder_header > rename it vlan_reorder_header to keep the naming clean. > > Fix up the skb->pkt_type early so we don't look at the packet > after adding the vlan tag, which guarantees we don't goof > and look at the wrong field. > > Use a simple if statement instead of a complicated switch > statement to decided that we need to increment rx_stats > for a multicast packet. > > Hopefully at somepoint we will just declare the case where > VLAN_FLAG_REORDER_HDR is cleared as unsupported and remove > the code. Until then this keeps it working correctly. > > Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> > --- > include/linux/if_vlan.h | 25 ++++++++++++++++++++-- > net/8021q/vlan_core.c | 50 ++++++++++++++++++++++------------------------ > 2 files changed, 46 insertions(+), 29 deletions(-) > > diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h > index 290bd8a..821f0e3 100644 > --- a/include/linux/if_vlan.h > +++ b/include/linux/if_vlan.h > @@ -220,7 +220,7 @@ static inline int vlan_hwaccel_receive_skb(struct sk_buff *skb, > } > > /** > - * __vlan_put_tag - regular VLAN tag inserting > + * vlan_insert_tag - regular VLAN tag inserting > * @skb: skbuff to tag > * @vlan_tci: VLAN TCI to insert > * > @@ -229,8 +229,10 @@ static inline int vlan_hwaccel_receive_skb(struct sk_buff *skb, > * > * Following the skb_unshare() example, in case of error, the calling function > * doesn't have to worry about freeing the original skb. > + * > + * Does not change skb->protocol so this function can be used during receive. > */ > -static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb, u16 vlan_tci) > +static inline struct sk_buff *vlan_insert_tag(struct sk_buff *skb, u16 vlan_tci) > { > struct vlan_ethhdr *veth; > > @@ -250,8 +252,25 @@ static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb, u16 vlan_tci) > /* now, the TCI */ > veth->h_vlan_TCI = htons(vlan_tci); > > - skb->protocol = htons(ETH_P_8021Q); > + return skb; > +} > > +/** > + * __vlan_put_tag - regular VLAN tag inserting > + * @skb: skbuff to tag > + * @vlan_tci: VLAN TCI to insert > + * > + * Inserts the VLAN tag into @skb as part of the payload > + * Returns a VLAN tagged skb. If a new skb is created, @skb is freed. > + * > + * Following the skb_unshare() example, in case of error, the calling function > + * doesn't have to worry about freeing the original skb. > + */ > +static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb, u16 vlan_tci) > +{ > + skb = vlan_insert_tag(skb, vlan_tci); > + if (skb) > + skb->protocol = htons(ETH_P_8021Q); > return skb; > } > > diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c > index 41495dc..735c818 100644 > --- a/net/8021q/vlan_core.c > +++ b/net/8021q/vlan_core.c > @@ -23,6 +23,20 @@ bool vlan_do_receive(struct sk_buff **skbp) > return false; > > skb->dev = vlan_dev; > + if (skb->pkt_type == PACKET_OTHERHOST) { > + /* Our lower layer thinks this is not local, let's make sure. > + * This allows the VLAN to have a different MAC than the > + * underlying device, and still route correctly. */ > + if (!compare_ether_addr(eth_hdr(skb)->h_dest, > + vlan_dev->dev_addr)) > + skb->pkt_type = PACKET_HOST; > + } > + > + if (unlikely(!(vlan_dev_info(vlan_dev)->flags & VLAN_FLAG_REORDER_HDR))) { > + skb = *skbp = vlan_insert_tag(skb, skb->vlan_tci); > + if (!skb) > + return false; > + } > skb->priority = vlan_get_ingress_priority(vlan_dev, skb->vlan_tci); > skb->vlan_tci = 0; > > @@ -31,22 +45,8 @@ bool vlan_do_receive(struct sk_buff **skbp) > u64_stats_update_begin(&rx_stats->syncp); > rx_stats->rx_packets++; > rx_stats->rx_bytes += skb->len; > - > - switch (skb->pkt_type) { > - case PACKET_BROADCAST: > - break; > - case PACKET_MULTICAST: > + if (skb->pkt_type == PACKET_MULTICAST) > rx_stats->rx_multicast++; > - break; > - case PACKET_OTHERHOST: > - /* Our lower layer thinks this is not local, let's make sure. > - * This allows the VLAN to have a different MAC than the > - * underlying device, and still route correctly. */ > - if (!compare_ether_addr(eth_hdr(skb)->h_dest, > - vlan_dev->dev_addr)) > - skb->pkt_type = PACKET_HOST; > - break; > - } > u64_stats_update_end(&rx_stats->syncp); > > return true; > @@ -89,17 +89,15 @@ gro_result_t vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp, > } > EXPORT_SYMBOL(vlan_gro_frags); > > -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) { > + /* Lifted from Gleb's VLAN code... */ > + memmove(skb->data - ETH_HLEN, > + skb->data - VLAN_ETH_HLEN, 12); > + skb->mac_header += VLAN_HLEN; > } > return skb; > } > @@ -161,7 +159,7 @@ struct sk_buff *vlan_untag(struct sk_buff *skb) > skb_pull_rcsum(skb, VLAN_HLEN); > vlan_set_encap_proto(skb, vhdr); > > - skb = vlan_check_reorder_header(skb); > + skb = vlan_reorder_header(skb); > if (unlikely(!skb)) > goto err_free; > > -- > 1.7.5.1.217.g4e3aa > > -- > 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 > -- 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
On Fri, Jun 3, 2011 at 11:34 AM, padmanabh ratnakar <pratnakarlx@gmail.com> wrote: > Doesnt __vlan_put_tag()/vlan_insert_tag() depend on skb->data pointing > to ethernet header. > Is'nt the skb->data pointing past ethernet header in netif_receive_skb(). > Am I missing anything? Yes, you are right. skb->data should be adjusted before feeding to vlan_insert_tag(), or we make vlan_insert_tag() rely on skb->mac_header instead.
From: Changli Gao <xiaosuo@gmail.com> Date: Fri, 3 Jun 2011 11:59:26 +0800 > On Fri, Jun 3, 2011 at 11:34 AM, padmanabh ratnakar > <pratnakarlx@gmail.com> wrote: >> Doesnt __vlan_put_tag()/vlan_insert_tag() depend on skb->data pointing >> to ethernet header. >> Is'nt the skb->data pointing past ethernet header in netif_receive_skb(). >> Am I missing anything? > > Yes, you are right. skb->data should be adjusted before feeding to > vlan_insert_tag(), or we make vlan_insert_tag() rely on > skb->mac_header instead. I told you guys this is not simple stuff :-) Even here we have 3 or 4 people who study this code all the time, and are still having trouble sorting out all of these details. -- 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
Thu, Jun 02, 2011 at 03:03:53PM CEST, ebiederm@xmission.com wrote: > >Testing of VLAN_FLAG_REORDER_HDR does not belong in vlan_untag >but rather in vlan_do_receive. Otherwise the vlan header >will not be properly put on the packet in the case of >vlan header accelleration. > >As we remove the check from vlan_check_reorder_header >rename it vlan_reorder_header to keep the naming clean. > >Fix up the skb->pkt_type early so we don't look at the packet >after adding the vlan tag, which guarantees we don't goof >and look at the wrong field. > >Use a simple if statement instead of a complicated switch >statement to decided that we need to increment rx_stats >for a multicast packet. > >Hopefully at somepoint we will just declare the case where >VLAN_FLAG_REORDER_HDR is cleared as unsupported and remove >the code. Until then this keeps it working correctly. Why we can't remove this right away? -- 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
On Thu, Jun 9, 2011 at 12:28 AM, Jiri Pirko <jpirko@redhat.com> wrote: > > Why we can't remove this right away? > I think the reason is this patch is a bug fix for the stable kernel, so we should not introduce new features or remove old features.
Thu, Jun 09, 2011 at 01:08:13AM CEST, xiaosuo@gmail.com wrote: >On Thu, Jun 9, 2011 at 12:28 AM, Jiri Pirko <jpirko@redhat.com> wrote: >> >> Why we can't remove this right away? >> > >I think the reason is this patch is a bug fix for the stable kernel, >so we should not introduce new features or remove old features. But for net-next if should be ok, right? What's the reason for possibility of removing the reorder flag? Why would user want do it? Thanks Jirka > >-- >Regards, >Changli Gao(xiaosuo@gmail.com) -- 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/include/linux/if_vlan.h b/include/linux/if_vlan.h index 290bd8a..821f0e3 100644 --- a/include/linux/if_vlan.h +++ b/include/linux/if_vlan.h @@ -220,7 +220,7 @@ static inline int vlan_hwaccel_receive_skb(struct sk_buff *skb, } /** - * __vlan_put_tag - regular VLAN tag inserting + * vlan_insert_tag - regular VLAN tag inserting * @skb: skbuff to tag * @vlan_tci: VLAN TCI to insert * @@ -229,8 +229,10 @@ static inline int vlan_hwaccel_receive_skb(struct sk_buff *skb, * * Following the skb_unshare() example, in case of error, the calling function * doesn't have to worry about freeing the original skb. + * + * Does not change skb->protocol so this function can be used during receive. */ -static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb, u16 vlan_tci) +static inline struct sk_buff *vlan_insert_tag(struct sk_buff *skb, u16 vlan_tci) { struct vlan_ethhdr *veth; @@ -250,8 +252,25 @@ static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb, u16 vlan_tci) /* now, the TCI */ veth->h_vlan_TCI = htons(vlan_tci); - skb->protocol = htons(ETH_P_8021Q); + return skb; +} +/** + * __vlan_put_tag - regular VLAN tag inserting + * @skb: skbuff to tag + * @vlan_tci: VLAN TCI to insert + * + * Inserts the VLAN tag into @skb as part of the payload + * Returns a VLAN tagged skb. If a new skb is created, @skb is freed. + * + * Following the skb_unshare() example, in case of error, the calling function + * doesn't have to worry about freeing the original skb. + */ +static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb, u16 vlan_tci) +{ + skb = vlan_insert_tag(skb, vlan_tci); + if (skb) + skb->protocol = htons(ETH_P_8021Q); return skb; } diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c index 41495dc..735c818 100644 --- a/net/8021q/vlan_core.c +++ b/net/8021q/vlan_core.c @@ -23,6 +23,20 @@ bool vlan_do_receive(struct sk_buff **skbp) return false; skb->dev = vlan_dev; + if (skb->pkt_type == PACKET_OTHERHOST) { + /* Our lower layer thinks this is not local, let's make sure. + * This allows the VLAN to have a different MAC than the + * underlying device, and still route correctly. */ + if (!compare_ether_addr(eth_hdr(skb)->h_dest, + vlan_dev->dev_addr)) + skb->pkt_type = PACKET_HOST; + } + + if (unlikely(!(vlan_dev_info(vlan_dev)->flags & VLAN_FLAG_REORDER_HDR))) { + skb = *skbp = vlan_insert_tag(skb, skb->vlan_tci); + if (!skb) + return false; + } skb->priority = vlan_get_ingress_priority(vlan_dev, skb->vlan_tci); skb->vlan_tci = 0; @@ -31,22 +45,8 @@ bool vlan_do_receive(struct sk_buff **skbp) u64_stats_update_begin(&rx_stats->syncp); rx_stats->rx_packets++; rx_stats->rx_bytes += skb->len; - - switch (skb->pkt_type) { - case PACKET_BROADCAST: - break; - case PACKET_MULTICAST: + if (skb->pkt_type == PACKET_MULTICAST) rx_stats->rx_multicast++; - break; - case PACKET_OTHERHOST: - /* Our lower layer thinks this is not local, let's make sure. - * This allows the VLAN to have a different MAC than the - * underlying device, and still route correctly. */ - if (!compare_ether_addr(eth_hdr(skb)->h_dest, - vlan_dev->dev_addr)) - skb->pkt_type = PACKET_HOST; - break; - } u64_stats_update_end(&rx_stats->syncp); return true; @@ -89,17 +89,15 @@ gro_result_t vlan_gro_frags(struct napi_struct *napi, struct vlan_group *grp, } EXPORT_SYMBOL(vlan_gro_frags); -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) { + /* Lifted from Gleb's VLAN code... */ + memmove(skb->data - ETH_HLEN, + skb->data - VLAN_ETH_HLEN, 12); + skb->mac_header += VLAN_HLEN; } return skb; } @@ -161,7 +159,7 @@ struct sk_buff *vlan_untag(struct sk_buff *skb) skb_pull_rcsum(skb, VLAN_HLEN); vlan_set_encap_proto(skb, vhdr); - skb = vlan_check_reorder_header(skb); + skb = vlan_reorder_header(skb); if (unlikely(!skb)) goto err_free;
Testing of VLAN_FLAG_REORDER_HDR does not belong in vlan_untag but rather in vlan_do_receive. Otherwise the vlan header will not be properly put on the packet in the case of vlan header accelleration. As we remove the check from vlan_check_reorder_header rename it vlan_reorder_header to keep the naming clean. Fix up the skb->pkt_type early so we don't look at the packet after adding the vlan tag, which guarantees we don't goof and look at the wrong field. Use a simple if statement instead of a complicated switch statement to decided that we need to increment rx_stats for a multicast packet. Hopefully at somepoint we will just declare the case where VLAN_FLAG_REORDER_HDR is cleared as unsupported and remove the code. Until then this keeps it working correctly. Signed-off-by: Eric W. Biederman <ebiederm@xmission.com> --- include/linux/if_vlan.h | 25 ++++++++++++++++++++-- net/8021q/vlan_core.c | 50 ++++++++++++++++++++++------------------------ 2 files changed, 46 insertions(+), 29 deletions(-)