[nf-next,3/3] netfilter: bridge: copy back VLAN header for bridge packet queued to userspace

From: stephane <stephane.ml.bryant@gmail.com>

For bridge packets queued to userspace, this uses the skb tci info
to reinstate the VLAN header, and conversely parses and removes it
to fill the tci info on the way back.

Signed-off-by: Stephane Bryant <stephane.ml.bryant@gmail.com>
---
 net/netfilter/nfnetlink_queue.c | 72 ++++++++++++++++++++++++++++++-----------
 1 file changed, 54 insertions(+), 18 deletions(-)

Stephane Bryant <stephane.ml.bryant@gmail.com> wrote:
> From: stephane <stephane.ml.bryant@gmail.com>
> 
> For bridge packets queued to userspace, this uses the skb tci info
> to reinstate the VLAN header, and conversely parses and removes it
> to fill the tci info on the way back.
> -			 * it gets copied in
> -			 */
>  			mac_header_len =
>  				(int)(entskb->data - skb_mac_header(entskb));
> -			skb_push(entskb, mac_header_len);
> +			if (skb_vlan_tag_present(entskb))
> +				vlan_len = VLAN_HLEN;

I wondered if we could use the saveroute and reroute hooks in the nf
afinfo to perform the push/pull.

It would keep the bridge specific parts out of the generic code.

Thanks for working on this!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Florian Westphal <fw@strlen.de> wrote:
> Stephane Bryant <stephane.ml.bryant@gmail.com> wrote:
> > From: stephane <stephane.ml.bryant@gmail.com>
> > 
> > For bridge packets queued to userspace, this uses the skb tci info
> > to reinstate the VLAN header, and conversely parses and removes it
> > to fill the tci info on the way back.
> > -			 * it gets copied in
> > -			 */
> >  			mac_header_len =
> >  				(int)(entskb->data - skb_mac_header(entskb));
> > -			skb_push(entskb, mac_header_len);
> > +			if (skb_vlan_tag_present(entskb))
> > +				vlan_len = VLAN_HLEN;
> 
> I wondered if we could use the saveroute and reroute hooks in the nf
> afinfo to perform the push/pull.
> 
> It would keep the bridge specific parts out of the generic code.

Addendum: If its not possible I'd prefer to add afinfo helpers for it to
keep this out of the generic part.

F.e. we will likely also want netdev family support later on.

As for complications wrt. nf_bridge_adjust_skb_data() (the software
segmentation part) I think the best way would be to reject attempts to
bind a queue for families other than NFPROTO_IPV4|6 without
NFQA_CFG_F_GSO flag present.

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Jan 15, 2016 at 09:48:54AM +0100, Stephane Bryant wrote:
> From: stephane <stephane.ml.bryant@gmail.com>
> 
> For bridge packets queued to userspace, this uses the skb tci info
> to reinstate the VLAN header, and conversely parses and removes it
> to fill the tci info on the way back.
> 
> Signed-off-by: Stephane Bryant <stephane.ml.bryant@gmail.com>
> ---
>  net/netfilter/nfnetlink_queue.c | 72 ++++++++++++++++++++++++++++++-----------
>  1 file changed, 54 insertions(+), 18 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index b299236..07723f9 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -548,7 +548,26 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  
>  		nla = (struct nlattr *)skb_put(skb, sizeof(*nla));
>  		nla->nla_type = NFQA_PAYLOAD;
> -		nla->nla_len = nla_attr_size(data_len);
> +		nla->nla_len =
> +			nla_attr_size(data_len + mac_header_len + vlan_len);
> +		if (mac_header_len > 0) {
> +			unsigned char *mac_header;
> +
> +			mac_header = skb_put(skb, mac_header_len + vlan_len);
> +			memcpy(mac_header, skb_mac_header(entskb),
> +			       mac_header_len);
> +			if (vlan_len > 0) {
> +				struct vlan_ethhdr *veth =
> +					(struct vlan_ethhdr *)mac_header;
> +
> +				u16 proto = veth->h_vlan_proto;
> +
> +				veth->h_vlan_proto = entskb->vlan_proto;
> +				veth->h_vlan_TCI =
> +					htons(skb_vlan_tag_get(entskb));
> +				veth->h_vlan_encapsulated_proto = proto;
> +			}
> +		}

For the specific case of nfnetlink_queue, I would expose the vlan
information through a new netlink attribute NFQA_VLAN (similar to what
we do for NFQA_HWADDR for the layer 3).

>  		if (skb_zerocopy(skb, entskb, data_len, hlen))
>  			goto nla_put_failure;
> @@ -556,9 +575,6 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>  
>  	nlh->nlmsg_len = skb->len;
>  
> -	if (mac_header_len > 0)
> -		skb_pull(entskb, mac_header_len);
> -
>  	return skb;
>  
>  nla_put_failure:
> @@ -1087,24 +1103,44 @@ static int nfqnl_recv_verdict(struct net *net, struct sock *ctnl,
>  
>  	if (nfqa[NFQA_PAYLOAD]) {
>  		u16 payload_len = nla_len(nfqa[NFQA_PAYLOAD]);
> +		unsigned char *payload = nla_data(nfqa[NFQA_PAYLOAD]);
>  		int diff = 0;
>  		int mac_header_len = 0;
> +
>  #if IS_ENABLED(CONFIG_BRIDGE_NETFILTER)
>  		if ((entry->state.pf == PF_BRIDGE) &&
>  		    (entry->skb->dev &&
>  		     (entry->skb->data > skb_mac_header(entry->skb)))) {
> -			/* push back the mac header into the data so that
> -			 * it gets copied in before mangling
> -			 */
> -			mac_header_len = (int)(entry->skb->data -
> -					       skb_mac_header(entry->skb));
> +			struct vlan_ethhdr *veth =
> +				(struct vlan_ethhdr *)payload;
> +
> +			mac_header_len = (int)
> +				(entry->skb->data - skb_mac_header(entry->skb));
> +			/* push back mac header before mangling */
>  			skb_push(entry->skb, mac_header_len);
> -	}
> +			/* check for VLAN in NFQA_PAYLOAD */
> +			if ((payload_len >= VLAN_ETH_HLEN) &&
> +			    ((veth->h_vlan_proto == htons(ETH_P_8021Q)) ||
> +			     (veth->h_vlan_proto == htons(ETH_P_8021AD)))) {
> +				entry->skb->vlan_proto = veth->h_vlan_proto;
> +				entry->skb->vlan_tci =
> +					ntohs(veth->h_vlan_TCI) |
> +					VLAN_TAG_PRESENT;
> +				memmove(payload + VLAN_HLEN, payload,
> +					2 * ETH_ALEN);
> +				entry->skb->protocol =
> +					veth->h_vlan_encapsulated_proto;
> +				payload += VLAN_HLEN;
> +				payload_len -= VLAN_HLEN;
> +			} else {
> +				entry->skb->vlan_tci &= ~VLAN_TAG_PRESENT;
> +				entry->skb->protocol = veth->h_vlan_proto;
> +			}
> +		}

I'm awar it's more work, but it would be good to reduce ifdef pollution
by placing all this bridge netfilter code wrapped into functions under
one single ifdef in this file to improve maintainability.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> For the specific case of nfnetlink_queue, I would expose the vlan
> information through a new netlink attribute NFQA_VLAN (similar to what
> we do for NFQA_HWADDR for the layer 3).

If we do this I think it does make sense to consider putting
the entire L2 mac header under its own attr too.

This is especially good if we'd later add support for NETDEV
family.  Since drivers already pull the L2 header userspace
would not need to handle arbirary L2 protocols.

> > +				payload += VLAN_HLEN;
> > +				payload_len -= VLAN_HLEN;
> > +			} else {
> > +				entry->skb->vlan_tci &= ~VLAN_TAG_PRESENT;
> > +				entry->skb->protocol = veth->h_vlan_proto;
> > +			}
> > +		}
> 
> I'm awar it's more work, but it would be good to reduce ifdef pollution
> by placing all this bridge netfilter code wrapped into functions under
> one single ifdef in this file to improve maintainability.

Right, but for anything family specifiy it would be even better to push
it into nf afinfo. In case thats too much work or too cumbersome (e.g.
because you'd need 12 function arguments ...) then the ifdef-wrapped
helper is fine of course.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Jan 15, 2016 at 03:04:12PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > For the specific case of nfnetlink_queue, I would expose the vlan
> > information through a new netlink attribute NFQA_VLAN (similar to what
> > we do for NFQA_HWADDR for the layer 3).
> 
> If we do this I think it does make sense to consider putting
> the entire L2 mac header under its own attr too.
> 
> This is especially good if we'd later add support for NETDEV
> family.  Since drivers already pull the L2 header userspace
> would not need to handle arbirary L2 protocols.

Good point, fine with me.

> > > +				payload += VLAN_HLEN;
> > > +				payload_len -= VLAN_HLEN;
> > > +			} else {
> > > +				entry->skb->vlan_tci &= ~VLAN_TAG_PRESENT;
> > > +				entry->skb->protocol = veth->h_vlan_proto;
> > > +			}
> > > +		}
> > 
> > I'm awar it's more work, but it would be good to reduce ifdef pollution
> > by placing all this bridge netfilter code wrapped into functions under
> > one single ifdef in this file to improve maintainability.
> 
> Right, but for anything family specifiy it would be even better to push
> it into nf afinfo. In case thats too much work or too cumbersome (e.g.
> because you'd need 12 function arguments ...) then the ifdef-wrapped
> helper is fine of course.

I'm fine with pushing this info afinfo if it fits fine there.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

I will rework the patches taking all these remarks into consideration.

Regarding the entire L2 header attribute. I assume it would be a
separate attribute from the existing NFQA_HWADDR? or would it make sense
to collapse these 2 into one (potentially with helpers to access each
relevant field) to avoid duplicating information (and redundancy, and
potential discrepancies)?

On 01/15/2016 05:33 PM, Pablo Neira Ayuso wrote:
> On Fri, Jan 15, 2016 at 03:04:12PM +0100, Florian Westphal wrote:
>> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>>> For the specific case of nfnetlink_queue, I would expose the vlan
>>> information through a new netlink attribute NFQA_VLAN (similar to what
>>> we do for NFQA_HWADDR for the layer 3).
>>
>> If we do this I think it does make sense to consider putting
>> the entire L2 mac header under its own attr too.
>>
>> This is especially good if we'd later add support for NETDEV
>> family.  Since drivers already pull the L2 header userspace
>> would not need to handle arbirary L2 protocols.
> 
> Good point, fine with me.
> 
>>>> +				payload += VLAN_HLEN;
>>>> +				payload_len -= VLAN_HLEN;
>>>> +			} else {
>>>> +				entry->skb->vlan_tci &= ~VLAN_TAG_PRESENT;
>>>> +				entry->skb->protocol = veth->h_vlan_proto;
>>>> +			}
>>>> +		}
>>>
>>> I'm awar it's more work, but it would be good to reduce ifdef pollution
>>> by placing all this bridge netfilter code wrapped into functions under
>>> one single ifdef in this file to improve maintainability.
>>
>> Right, but for anything family specifiy it would be even better to push
>> it into nf afinfo. In case thats too much work or too cumbersome (e.g.
>> because you'd need 12 function arguments ...) then the ifdef-wrapped
>> helper is fine of course.
> 
> I'm fine with pushing this info afinfo if it fits fine there.
> 

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

stéphane bryant <stephane.ml.bryant@gmail.com> wrote:

Please don't top-post, thanks.

> Regarding the entire L2 header attribute. I assume it would be a
> separate attribute from the existing NFQA_HWADDR?

Yes, it would just contain mac header start to skb->data, i.e. without
VLAN header.  VLAN header would go into a new attribute too.

> Or would it make sense
> to collapse these 2 into one (potentially with helpers to access each
> relevant field) to avoid duplicating information (and redundancy, and
> potential discrepancies)?

Yes but we cannot do it due to backwards compatibility.
We *can* omit NFQA_HWADDR in the family == NFPROTO_BRIDGE case of
course.

Thanks for working on this,
Florian
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 01/15/2016 03:04 PM, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> For the specific case of nfnetlink_queue, I would expose the vlan
>> information through a new netlink attribute NFQA_VLAN (similar to what
>> we do for NFQA_HWADDR for the layer 3).
> 
> If we do this I think it does make sense to consider putting
> the entire L2 mac header under its own attr too.
> 
> This is especially good if we'd later add support for NETDEV
> family.  Since drivers already pull the L2 header userspace
> would not need to handle arbirary L2 protocols.
> 
>>> +				payload += VLAN_HLEN;
>>> +				payload_len -= VLAN_HLEN;
>>> +			} else {
>>> +				entry->skb->vlan_tci &= ~VLAN_TAG_PRESENT;
>>> +				entry->skb->protocol = veth->h_vlan_proto;
>>> +			}
>>> +		}
>>
>> I'm awar it's more work, but it would be good to reduce ifdef pollution
>> by placing all this bridge netfilter code wrapped into functions under
>> one single ifdef in this file to improve maintainability.
> 
> Right, but for anything family specifiy it would be even better to push
> it into nf afinfo. In case thats too much work or too cumbersome (e.g.
> because you'd need 12 function arguments ...) then the ifdef-wrapped
> helper is fine of course.

As the nf_afinfo saveroute/reroute hooks are called on the original
packet skb, at a location where netlink attributes are not in existence,
it only seems possible to use these hooks to hide the L2 code if we
pull-in/pull out the L2 header into/from the original skb, and forego
the new attributes -- which is fine by me as it is precisely what i was
doing in the original patches (albeit in a different location).
If we use new netlink attributes (NFQA_VLAN, NFQA_L2HDR) we will have to
wrap them in a #ifder helper, it seems.
I can go either way.

> .
> 

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

stéphane bryant <stephane.ml.bryant@gmail.com> wrote:
> On 01/15/2016 03:04 PM, Florian Westphal wrote:
> > Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >> For the specific case of nfnetlink_queue, I would expose the vlan
> >> information through a new netlink attribute NFQA_VLAN (similar to what
> >> we do for NFQA_HWADDR for the layer 3).
> > 
> > If we do this I think it does make sense to consider putting
> > the entire L2 mac header under its own attr too.
> > 
> > This is especially good if we'd later add support for NETDEV
> > family.  Since drivers already pull the L2 header userspace
> > would not need to handle arbirary L2 protocols.
> > 
> >>> +				payload += VLAN_HLEN;
> >>> +				payload_len -= VLAN_HLEN;
> >>> +			} else {
> >>> +				entry->skb->vlan_tci &= ~VLAN_TAG_PRESENT;
> >>> +				entry->skb->protocol = veth->h_vlan_proto;
> >>> +			}
> >>> +		}
> >>
> >> I'm awar it's more work, but it would be good to reduce ifdef pollution
> >> by placing all this bridge netfilter code wrapped into functions under
> >> one single ifdef in this file to improve maintainability.
> > 
> > Right, but for anything family specifiy it would be even better to push
> > it into nf afinfo. In case thats too much work or too cumbersome (e.g.
> > because you'd need 12 function arguments ...) then the ifdef-wrapped
> > helper is fine of course.
> 
> As the nf_afinfo saveroute/reroute hooks are called on the original
> packet skb, at a location where netlink attributes are not in existence,
> it only seems possible to use these hooks to hide the L2 code if we
> pull-in/pull out the L2 header into/from the original skb, and forego
> the new attributes -- which is fine by me as it is precisely what i was
> doing in the original patches (albeit in a different location).

Yes.  Problem with push/pull is that it will be impossible to do packet
rewriting of the l2 header since we'd have to pull a different amount.

> If we use new netlink attributes (NFQA_VLAN, NFQA_L2HDR) we will have to
> wrap them in a #ifder helper, it seems.

ifdef helper is fine.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html