netdev - Re: [PATCH net-next 2/2] net: dsa: lan9303: Learn addresses on CPU port when bridged

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b0da2547-69d3-b323-6918-1022e112f7db@egil-hjelmeland.no>
Date:   Wed, 25 Oct 2017 12:54:18 +0200
From:   Egil Hjelmeland <privat@...l-hjelmeland.no>
To:     Woojung.Huh@...rochip.com, andrew@...n.ch,
        vivien.didelot@...oirfairelinux.com, f.fainelli@...il.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] net: dsa: lan9303: Learn addresses on CPU
 port when bridged

Hi Woojung!

On 24. okt. 2017 19:18, Woojung.Huh@...rochip.com wrote:
> Hi Egil,
> 
>> +static inline int lan9303_tx_use_arl(struct dsa_port *dp, u8 *dest_addr)
>> +{
>> +	struct lan9303 *chip = dp->ds->priv;
>> +
>> +	return chip->is_bridged && !ether_addr_equal(dest_addr,
>> eth_stp_addr);
>> +}
>>
>>   static struct sk_buff *lan9303_xmit(struct sk_buff *skb, struct net_device
>> *dev)
>>   {
>> @@ -62,7 +80,10 @@ static struct sk_buff *lan9303_xmit(struct sk_buff *skb,
>> struct net_device *dev)
>>
>>   	lan9303_tag = (u16 *)(skb->data + 2 * ETH_ALEN);
>>   	lan9303_tag[0] = htons(ETH_P_8021Q);
>> -	lan9303_tag[1] = htons(dp->index | BIT(4));
>> +	lan9303_tag[1] = lan9303_tx_use_arl(dp, skb->data) ?
> 
> How about using skb_mac_header(skb) than skb->data?
> 
>> +				LAN9303_TAG_TX_USE_ALR :
>> +				dp->index |
> 

I am not the expert here.

I see that skb_mac_header() is (skb->head + skb->mac_header). So it will
cost a few nano seconds per packet. Not the end of the world though.
But I see that other net/dsa/tag_*.c use skb->data, assuming that
skb->data point to mac header.

Anyway, it may be an idea to decrement skb->mac_header, in case the
master interface driver uses it? What about skb->mac_len?

If to use skb_mac_header() at all, I would replace all use of skb->data,
like this:

---
/* provide 'LAN9303_TAG_LEN' bytes additional space */
skb_push(skb, LAN9303_TAG_LEN);

/* make room between MACs and Ether-Type */
memmove(skb_mac_header(skb) - LAN9303_TAG_LEN, skb_mac_header(skb),
		2 * ETH_ALEN);
skb->mac_header -= LAN9303_TAG_LEN;

lan9303_tag = (u16 *)(skb_mac_header(skb) + 2 * ETH_ALEN);
lan9303_tag[0] = htons(ETH_P_8021Q);
lan9303_tag[1] = lan9303_tx_use_arl(dp, skb_mac_header(skb)) ?
			LAN9303_TAG_TX_USE_ALR :
			dp->index | LAN9303_TAG_TX_STP_OVERRIDE;
lan9303_tag[1] = htons(lan9303_tag[1]);
---

But I will really like to hear the opinion from more people on this
before going down that road. Anyway, I think it would belong to a
separate patch.


> Thanks.
> Woojung
> 
> 

Regards
Egil