| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <BANLkTikjdz+LkavRiuqdDNS6+djAer-a0A@mail.gmail.com>
Date: Fri, 3 Jun 2011 09:04:12 +0530
From: padmanabh ratnakar <pratnakarlx@...il.com>
To: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: David Miller <davem@...emloft.net>,
shemminger@...ux-foundation.org, greearb@...delatech.com,
nicolas.2p.debian@...il.com, jpirko@...hat.com, xiaosuo@...il.com,
netdev@...r.kernel.org, kaber@...sh.net, fubar@...ibm.com,
eric.dumazet@...il.com, andy@...yhouse.net, jesse@...ira.com
Subject: Re: [PATCH] vlan: Fix the ingress VLAN_FLAG_REORDER_HDR check v2
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@...ssion.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@...ssion.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@...r.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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists