Re: [PATCH v10 nf-next 2/3] netfilter: nft_chain_filter: Add bridge double vlan and pppoe

[Eric Woudstra <ericwouds@gmail.com>]

On 3/19/25 12:04 AM, Pablo Neira Ayuso wrote:
> Hi,
> 
> On Sat, Mar 15, 2025 at 09:00:32PM +0100, Eric Woudstra wrote:
>> This adds the capability to evaluate 802.1ad, QinQ, PPPoE and PPPoE-in-Q
>> packets in the bridge filter chain.
>>
>> Reviewed-by: Nikolay Aleksandrov <razor@blackwall.org>
>> Signed-off-by: Eric Woudstra <ericwouds@gmail.com>
>> ---
>>  net/netfilter/nft_chain_filter.c | 20 +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
>> index 19a553550c76..7c7080c1a67d 100644
>> --- a/net/netfilter/nft_chain_filter.c
>> +++ b/net/netfilter/nft_chain_filter.c
>> @@ -232,11 +232,27 @@ nft_do_chain_bridge(void *priv,
>>  		    struct sk_buff *skb,
>>  		    const struct nf_hook_state *state)
>>  {
>> +	struct ethhdr *ethh = eth_hdr(skb);
>>  	struct nft_pktinfo pkt;
>> +	int thoff;
>>  
>>  	nft_set_pktinfo(&pkt, skb, state);
>>  
>> -	switch (eth_hdr(skb)->h_proto) {
>> +	switch (ethh->h_proto) {
>> +	case htons(ETH_P_PPP_SES):
>> +		thoff = PPPOE_SES_HLEN;
>> +		ethh += thoff;
> 
> This pointer arithmetics does not look correct, ethh is struct ethhdr,
> neither void nor char.
> 
>> +		break;
>> +	case htons(ETH_P_8021Q):
>> +		thoff = VLAN_HLEN;
>> +		ethh += thoff;
> 
> Same here.
> 
>> +		break;
>> +	default:
>> +		thoff = 0;
>> +		break;
>> +	}
>> +
>> +	switch (ethh->h_proto) {
> 
> This switch will match on the wrong offset.
> 
>>  	case htons(ETH_P_IP):
>>  		nft_set_pktinfo_ipv4_validate(&pkt);
>>  		break;
>> @@ -248,6 +264,8 @@ nft_do_chain_bridge(void *priv,
>>  		break;
>>  	}
>>  
>> +	pkt.thoff += thoff;
> 
> And only transport offset is adjusted here.
> 
>>  	return nft_do_chain(&pkt, priv);
>>  }
>>  
>> -- 
>> 2.47.1
>>

I will sort this out and send a new version after the merge window.