linux-kernel - Re: [PATCH net] af_packet: move notifier's packet_dev

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] [day] [month] [year] [list]

Message-ID: <aC4FYWEtxCw25Uwu@mini-arch>
Date: Wed, 21 May 2025 09:54:57 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
	davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
	horms@...nel.org, linux-kernel@...r.kernel.org,
	syzbot+b191b5ccad8d7a986286@...kaller.appspotmail.com
Subject: Re: [PATCH net] af_packet: move notifier's packet_dev_mc out of rcu
 critical section

On 05/20, Willem de Bruijn wrote:
> Jakub Kicinski wrote:
> > On Tue, 20 May 2025 22:41:30 -0400 Willem de Bruijn wrote:
> > > > @@ -4277,6 +4280,13 @@ static int packet_notifier(struct notifier_block *this,
> > > >  		}
> > > >  	}
> > > >  	rcu_read_unlock();
> > > > +
> > > > +	/* packet_dev_mc might grab instance locks so can't run under rcu */
> > > > +	list_for_each_entry_safe(ml, tmp, &mclist, remove_list) {
> > > > +		packet_dev_mc(dev, ml, -1);
> > > > +		kfree(ml);
> > > > +	}
> > > > +  
> > > 
> > > Just verifying my understanding of the not entirely obvious locking:
> > > 
> > > po->mclist modifications (add, del, flush, unregister) are all
> > > protected by the RTNL, not the RCU. The RCU only protects the sklist
> > > and by extension the sks on it. So moving the mclist operations out of
> > > the RCU is fine.
> > > 
> > > The delayed operation on the mclist entry is still within the RTNL
> > > from unregister_netdevice_notifier. Which matter as it protects not
> > > only the list, but also the actual operations in packet_dev_mc, such
> > > as inc/dec on dev->promiscuity and associated dev_change_rx_flags.
> > > And new packet_mclist.remove_list too.
> > 
> > Matches my understanding FWIW, but this will be a great addition 
> > to the commit message. Let's add it in v2..
> > 
> > > >  	return NOTIFY_DONE;
> > > >  }
> > > >  
> > > > diff --git a/net/packet/internal.h b/net/packet/internal.h
> > > > index d5d70712007a..1e743d0316fd 100644
> > > > --- a/net/packet/internal.h
> > > > +++ b/net/packet/internal.h
> > > > @@ -11,6 +11,7 @@ struct packet_mclist {
> > > >  	unsigned short		type;
> > > >  	unsigned short		alen;
> > > >  	unsigned char		addr[MAX_ADDR_LEN];
> > > > +	struct list_head	remove_list;  
> > > 
> > > INIT_LIST_HEAD on alloc in packet_mc_add?
> > 
> > Just to be clear this is an "entry node" not a "head node",
> > is it common to init "entry nodes"? 
> 
> I wasn't sure. A small sample from net/core showed that many do, e.g.,
> napi->poll_list. But not all, e.g., failover->list just calls
> list_add_tail immediately.
> 
> I suspect, and from that it seems, that it is safe to not explicitly
> initalize entry nodes if you know what you're doing / how they're
> used.
> 
> But whether that is actually intended to work, especially with more
> involved debugging (such as LIST_POISON) and invariant checking
> (__list_add_valid), I don't know.
> 
> I did not find any authoritative documentation that says you have too,
> so I guess it's fine. But not ideal.

I can add the initialization to be safe and add more info to the commit
message, thank you both for the feedback!