netdev - Re: DSA support for Marvell 88e6065 switch

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]

Date:   Wed, 5 Dec 2018 14:20:14 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Pavel Machek <pavel@....cz>,
        Lennert Buytenhek <buytenh@...tstofly.org>
Cc:     Andrew Lunn <andrew@...n.ch>, netdev@...r.kernel.org
Subject: Re: DSA support for Marvell 88e6065 switch

On 12/5/18 12:46 PM, Pavel Machek wrote:
> Hi!
> 
>>>>> But I'm running into problems with tagging code, and I guess I'd like
>>>>> some help understanding.
>>>>>
>>>>> tag_trailer: allocates new skb, then copies data around.
>>>>>
>>>>> tag_qca: does dev->stats.tx_packets++, and reuses existing skb.
>>>>>
>>>>> tag_brcm: reuses existing skb.
>>>
>>> Any idea why tag trailer allocates new skb,
>>
>> I wrote this code over 10 years ago, so I don't remember all that
>> well, but I think that it is because you have to do manual checksumming
>> of the packet, as there's no way to pass down the stack that you don't
>> want to checksum all the way down to the end of the data area (and you
>> don't want the tag to be included in the checksum), and so you want to
>> do that before you add the trailer tag, and you'll probably have to
>> reallocate the data area to be able to add the tag, and you probably
>> won't get an exclusive skb here anyway, so you might as well allocate
>> a new one.
> 
> Ok, thanks. Whether tag is checksummed or not is indeed an interesting question.
> 
>>> and what is going on with dev->stats.tx_packets++?
>>
>> trailer_xmit would be the hard_start_xmit function for the virtual
>> (slave) network interface, so this would be the right thing to do?
> 
> Some tag_*.c do this and some do not, so I'm still confused.

This is likely just historical, or we don't have an easy way to
determine whether the SKB transmitted succeeded or not because it was
reallocated, and therefore the tagging code must make sure statistics
are maintained. I suspect it is just historical and that tag module just
has not been updated to leverage the statistics accounting done a layer
above it within DSA.

> 
>>> 6065 indeed has some kind of "egress tagging mode" (with four
>>> options), but I have trouble understanding what it really does.
>>
>> What are the options?
> 
> "transmit unmodified", "transmit untagged", "transmit tagged" and
> "transmit all frames double tagged".

It seems to me that this applies to VLAN headers, not DSA/trailer
headers though not having the data sheet I should be flat out wrong.
-- 
Florian