netdev - Re: [PATCH net-next v5 3/5] netdevsim: forward skbs from one connected port to another

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, 3 Jan 2024 13:57:40 -0800
From: David Wei <dw@...idwei.uk>
To: Eric Dumazet <edumazet@...gle.com>
Cc: Jakub Kicinski <kuba@...nel.org>, Jiri Pirko <jiri@...nulli.us>,
 Sabrina Dubroca <sd@...asysnail.net>, netdev@...r.kernel.org,
 "David S. Miller" <davem@...emloft.net>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next v5 3/5] netdevsim: forward skbs from one
 connected port to another

On 2024-01-02 03:20, Eric Dumazet wrote:
> On Thu, Dec 28, 2023 at 2:46 AM David Wei <dw@...idwei.uk> wrote:
>>
>> Forward skbs sent from one netdevsim port to its connected netdevsim
>> port using dev_forward_skb, in a spirit similar to veth.
>>
>> Add a tx_dropped variable to struct netdevsim, tracking the number of
>> skbs that could not be forwarded using dev_forward_skb().
>>
>> The xmit() function accessing the peer ptr is protected by an RCU read
>> critical section. The rcu_read_lock() is functionally redundant as since
>> v5.0 all softirqs are implicitly RCU read critical sections; but it is
>> useful for human readers.
>>
>> If another CPU is concurrently in nsim_destroy(), then it will first set
>> the peer ptr to NULL. This does not affect any existing readers that
>> dereferenced a non-NULL peer. Then, in unregister_netdevice(), there is
>> a synchronize_rcu() before the netdev is actually unregistered and
>> freed. This ensures that any readers i.e. xmit() that got a non-NULL
>> peer will complete before the netdev is freed.
>>
>> Any readers after the RCU_INIT_POINTER() but before synchronize_rcu()
>> will dereference NULL, making it safe.
>>
>> The codepath to nsim_destroy() and nsim_create() takes both the newly
>> added nsim_dev_list_lock and rtnl_lock. This makes it safe with
>> concurrent calls to linking two netdevsims together.
>>
>> Signed-off-by: David Wei <dw@...idwei.uk>
>> ---
>>  drivers/net/netdevsim/netdev.c    | 21 ++++++++++++++++++---
>>  drivers/net/netdevsim/netdevsim.h |  1 +
>>  2 files changed, 19 insertions(+), 3 deletions(-)
>>
> 
> 
>> @@ -302,7 +318,6 @@ static void nsim_setup(struct net_device *dev)
>>         eth_hw_addr_random(dev);
>>
>>         dev->tx_queue_len = 0;
>> -       dev->flags |= IFF_NOARP;
> 
> This part seems to be unrelated to this patch ?

Hi Eric, I found that this change is needed for skb forwarding to work.
Would you prefer me splitting this change into its own patch?

> 
>>         dev->flags &= ~IFF_MULTICAST;
>>         dev->priv_flags |= IFF_LIVE_ADDR_CHANGE |
>>                            IFF_NO_QUEUE;