| Message ID | 20241017183140.43028-6-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | phonet: Convert all doit() and dumpit() to RCU. | expand |
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@amazon.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,
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)); + + 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,
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@amazon.com> --- net/phonet/pn_netlink.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)