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

["stéphane bryant" <stephane.ml.bryant@gmail.com>]

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.
>