Re: [PATCH 1/2] rtnetlink: use nlmsg_{multicast, unicast} instead of netlink_{broadcast,unicast}

Cong Wang
2021-07-15T04:22:06Z

On Tue, Jul 13, 2021 at 5:37 AM Yajun Deng <yajun.deng@linux.dev> wrote:
>
> It has a 'NETLINK_CB(' statement in nlmsg_multicast() and has 'if (err'
> in nlmsg_{multicast, unicast}, use nlmsg_{multicast, unicast} instead
> of netlink_{broadcast,unicast}. so the caller would not deal with the
> 'if (err >0 )' statement. Add the return value for nlmsg_multicast.
> As also, rename rtnetlink_send() to rtnl_send(), this makes style
> uniform.
>
> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
> ---
> include/linux/rtnetlink.h | 2 +-
> net/core/rtnetlink.c | 13 +++++++------
> 2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
> index bb9cb84114c1..60bef82e42ab 100644
> --- a/include/linux/rtnetlink.h
> +++ b/include/linux/rtnetlink.h
> @@ -9,7 +9,7 @@
> #include <linux/refcount.h>
> #include <uapi/linux/rtnetlink.h>
>
> -extern int rtnetlink_send(struct sk_buff *skb, struct net *net, u32 pid, u32 group, int echo);
> +extern int rtnl_send(struct sk_buff *skb, struct net *net, u32 pid, u32 group, int echo);
> extern int rtnl_unicast(struct sk_buff *skb, struct net *net, u32 pid);
> extern void rtnl_notify(struct sk_buff *skb, struct net *net, u32 pid,
> u32 group, struct nlmsghdr *nlh, gfp_t flags);
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index f6af3e74fc44..c081d607bb69 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -707,17 +707,18 @@ static int rtnl_link_fill(struct sk_buff *skb, const struct net_device *dev)
> return err;
> }
>
> -int rtnetlink_send(struct sk_buff *skb, struct net *net, u32 pid, unsigned int group, int echo)
> +int rtnl_send(struct sk_buff *skb, struct net *net, u32 pid, unsigned int group, int echo)
> {
> struct sock *rtnl = net->rtnl;
> int err = 0;
>
> - NETLINK_CB(skb).dst_group = group;
> - if (echo)
> + err = nlmsg_multicast(rtnl, skb, pid, group, GFP_KERNEL);
> +
> + if (echo) {
> refcount_inc(&skb->users);

You also moved this refcount_inc() down after nlmsg_multicast().
Are you sure it is safe?

And the name rtnl_send() is bad given that rtnl_unicast() follows it...

Thanks.