netdev - Re: [PATCH net-next] mctp: emit RTM_NEWADDR and RTM

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]

Date:   Wed, 15 Dec 2021 12:09:31 +0800
From:   Jeremy Kerr <jk@...econstruct.com.au>
To:     Matt Johnston <matt@...econstruct.com.au>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] mctp: emit RTM_NEWADDR and RTM_DELADDR

Hi Matt,

> Userspace can receive notification of MCTP address changes via
> RTNLGRP_MCTP_IFADDR rtnetlink multicast group.

Nice! A couple of minor things:

> +static void mctp_addr_notify(struct mctp_dev *mdev, mctp_eid_t eid, int msg_type,
> +                            struct sk_buff *req_skb, struct nlmsghdr *req_nlh)

Is it worthwhile keeping the portid argument here, rather than the
entire request skb? Might prevent some confusion with the notify skb.

(I'm OK if we settle on the req_skb approach too though, as it does save
a little repeated code in call sites)

> +{
> +       int rc;
> +       struct sk_buff *skb;
> +       struct net *net = dev_net(mdev->dev);
> +       u32 portid = NETLINK_CB(req_skb).portid;

Super minor, but: reverse Christmas tree here (and above)?

> +
> +       skb = nlmsg_new(mctp_addrinfo_size(), GFP_KERNEL);
> +       if (!skb) {
> +               rc = -ENOBUFS;
> +               goto out;
> +       }
> +
> +       rc = mctp_fill_addrinfo(skb, mdev, eid, msg_type,
> +                               portid, req_nlh->nlmsg_seq, 0);
> +       if (rc < 0)
> +               goto out;
> +
> +       rtnl_notify(skb, net, portid, RTNLGRP_MCTP_IFADDR, req_nlh,
> GFP_KERNEL);
> +out:
> +       if (rc < 0) {
> +               if (skb)
> +                       kfree_skb(skb);
> +               rtnl_set_sk_err(net, RTNLGRP_MCTP_IFADDR, rc);
> +       }

We don't need that `if (skb)` condition on the kfree_skb - Yang has
removed a bunch in 5cfe53cfeb1. We could also remove the rc check if we
use the goto-label only for errors. Maybe something like:

            rtnl_notify(skb, net, portid, RTNLGRP_MCTP_IFADDR, req_nlh, GFP_KERNEL);
            return;
    err:
            kfree_skb(skb);
            rtnl_set_sk_err(net, RTNLGRP_MCTP_IFADDR, rc);
    }

Cheers,


Jeremy