Re: [PATCH v1 net-next 5/9] phonet: Don't hold RTNL for getaddr_dumpit().

["Rémi Denis-Courmont" <remi@remlab.net>]

Le perjantaina 18. lokakuuta 2024, 20.16.29 EEST Kuniyuki Iwashima a écrit :
> From: "Rémi Denis-Courmont" <remi@remlab.net>
> Date: Thu, 17 Oct 2024 21:49:18 +0300
> 
> > > diff --git a/net/phonet/pn_netlink.c b/net/phonet/pn_netlink.c
> > > index 5996141e258f..14928fa04675 100644
> > > --- a/net/phonet/pn_netlink.c
> > > +++ b/net/phonet/pn_netlink.c
> > > @@ -127,14 +127,17 @@ static int fill_addr(struct sk_buff *skb, u32
> > > ifindex, u8 addr,
> > > 
> > >  static int getaddr_dumpit(struct sk_buff *skb, struct netlink_callback
> > >  *cb)
> > > 
> > > {
> > > +	int addr_idx = 0, addr_start_idx = cb->args[1];
> > > +	int dev_idx = 0, dev_start_idx = cb->args[0];
> > > 
> > >  	struct phonet_device_list *pndevs;
> > >  	struct phonet_device *pnd;
> > > 
> > > -	int dev_idx = 0, dev_start_idx = cb->args[0];
> > > -	int addr_idx = 0, addr_start_idx = cb->args[1];
> > > +	int err = 0;
> > > 
> > >  	pndevs = phonet_device_list(sock_net(skb->sk));
> > > 
> > > +
> > > 
> > >  	rcu_read_lock();
> > >  	list_for_each_entry_rcu(pnd, &pndevs->list, list) {
> > > 
> > > +		DECLARE_BITMAP(addrs, 64);
> > > 
> > >  		u8 addr;
> > >  		
> > >  		if (dev_idx > dev_start_idx)
> > > 
> > > @@ -143,23 +146,26 @@ static int getaddr_dumpit(struct sk_buff *skb,
> > > struct
> > > netlink_callback *cb) continue;
> > > 
> > >  		addr_idx = 0;
> > > 
> > > -		for_each_set_bit(addr, pnd->addrs, 64) {
> > > +		memcpy(addrs, pnd->addrs, sizeof(pnd->addrs));
> > 
> > Is that really safe? Are we sure that the bit-field writers are atomic
> > w.r.t. memcpy() on all platforms? If READ_ONCE is needed for an integer,
> > using memcpy() seems sketchy, TBH.
> 
> I think bit-field read/write need not be atomic here because even
> if a data-race happens, for_each_set_bit() iterates each bit, which
> is the real data, regardless of whether data-race happened or not.

Err, it looks to me that a corrupt bit would lead to the index getting corrupt 
and addresses getting skipped or repeated. AFAICT, the RTNL lock is still 
needed here.

-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/