netdev - Re: [PATCH net-next v4 10/10] tsnep: Support XDP BPF program setup

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: <8e6fae4d-83f4-e31c-2274-208e27e7b156@engleder-embedded.com>
Date:   Wed, 11 Jan 2023 20:11:44 +0100
From:   Gerhard Engleder <gerhard@...leder-embedded.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        Alexander H Duyck <alexander.duyck@...il.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, edumazet@...gle.com,
        pabeni@...hat.com
Subject: Re: [PATCH net-next v4 10/10] tsnep: Support XDP BPF program setup

On 11.01.23 01:12, Jakub Kicinski wrote:
> On Tue, 10 Jan 2023 22:38:04 +0100 Gerhard Engleder wrote:
>>> As I called out earlier the __TSNEP_DOWN is just !IFF_UP so you don't
>>> need that bit.
>>>
>>> The fact that netif_carrier_off is here also points out the fact that
>>> the code in the Tx path isn't needed regarding __TSNEP_DOWN and you can
>>> probably just check netif_carrier_ok if you need the check.
>>
>> tsnep_netdev_close() is called directly during bpf prog setup (see
>> tsnep_xdp_setup_prog() in this commit). If the following
>> tsnep_netdev_open() call fails, then this flag signals that the device
>> is already down and nothing needs to be cleaned up if
>> tsnep_netdev_close() is called later (because IFF_UP is still set).
> 
> TBH we've been pushing pretty hard for a while now to stop people
> from implementing the:
> 
> 	close()
> 	change config
> 	open()
> 
> sort of reconfiguration. I did that myself when I was a was
> implementing my first Ethernet driver and DaveM nacked the change.
> Must have been a decade ago.
> 
> Imagine you're working on a remote box via SSH and the box is under
> transient memory pressure. Allocations fail, we don't want the machine
> to fall off the network :(

I agree with you that this pattern is bad. Most XDP BPF program setup do
it like that, but this is of course no valid argument.

In the last review round I made the following suggestion (but got no
reply so far):

What about always using 'XDP_PACKET_HEADROOM' as offset in the RX
buffer? The offset 'NET_SKB_PAD + NET_IP_ALIGN' would not even be used
if XDP is not enabled. Changing this offset is the only task to be done
at the first XDP BFP prog setup call. By always using this offset
no

	close()
	change config
	open()

pattern is needed. As a result no handling for failed open() is needed
and __TSNEP_DOWN is not needed. Simpler code with less problems in my
opinion.

The only problem could be that NET_IP_ALIGN is not used, but
NET_IP_ALIGN is 0 anyway on the two platforms (x86, arm64) where this
driver is used.

Gerhard