netdev - Re: [PATCH v1 net-next 5/9] phonet: Don't hold RTNL for getaddr

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: <2341285.ElGaqSPkdT@basile.remlab.net>
Date: Thu, 17 Oct 2024 21:49:18 +0300
From: Rémi Denis-Courmont <remi@...lab.net>
To: Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org
Subject:
 Re: [PATCH v1 net-next 5/9] phonet: Don't hold RTNL for getaddr_dumpit().

Le torstaina 17. lokakuuta 2024, 21.31.36 EEST Kuniyuki Iwashima a écrit :
> getaddr_dumpit() already relies on RCU and does not need RTNL.
> 
> Let's use READ_ONCE() for ifindex and register getaddr_dumpit()
> with RTNL_FLAG_DUMP_UNLOCKED.
> 
> While at it, the retval of getaddr_dumpit() is changed to combine
> NLMSG_DONE and save recvmsg() as done in 58a4ff5d77b1 ("phonet: no
> longer hold RTNL in route_dumpit()").
> 
> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> ---
>  net/phonet/pn_netlink.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> 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.

> +
> +		for_each_set_bit(addr, addrs, 64) {
>  			if (addr_idx++ < addr_start_idx)
>  				continue;
> 
> -			if (fill_addr(skb, pnd->netdev->ifindex, addr 
<< 2,
> -					 NETLINK_CB(cb-
>skb).portid,
> -					cb->nlh->nlmsg_seq, 
RTM_NEWADDR) < 0)
> +			err = fill_addr(skb, READ_ONCE(pnd->netdev-
>ifindex),
> +					addr << 2, 
NETLINK_CB(cb->skb).portid,
> +					cb->nlh->nlmsg_seq, 
RTM_NEWADDR);
> +			if (err < 0)
>  				goto out;
>  		}
>  	}
> -
>  out:
>  	rcu_read_unlock();
> +
>  	cb->args[0] = dev_idx;
>  	cb->args[1] = addr_idx;
> 
> -	return skb->len;
> +	return err;
>  }
> 
>  /* Routes handling */
> @@ -298,7 +304,7 @@ static const struct rtnl_msg_handler
> phonet_rtnl_msg_handlers[] __initdata_or_mo {.owner = THIS_MODULE,
> .protocol = PF_PHONET, .msgtype = RTM_DELADDR, .doit = addr_doit, .flags =
> RTNL_FLAG_DOIT_UNLOCKED},
>  	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = 
RTM_GETADDR,
> -	 .dumpit = getaddr_dumpit},
> +	 .dumpit = getaddr_dumpit, .flags = RTNL_FLAG_DUMP_UNLOCKED},
>  	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = 
RTM_NEWROUTE,
>  	 .doit = route_doit},
>  	{.owner = THIS_MODULE, .protocol = PF_PHONET, .msgtype = 
RTM_DELROUTE,


-- 
Rémi Denis-Courmont
http://www.remlab.net/