netdev - Re: [PATCH net-next] vrf: Add ip rules at vrf device create

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]

Message-ID: <5666E70F.2010307@cumulusnetworks.com>
Date:	Tue, 8 Dec 2015 07:19:59 -0700
From:	David Ahern <dsa@...ulusnetworks.com>
To:	Nikolay Aleksandrov <nikolay@...ulusnetworks.com>,
	netdev@...r.kernel.org
Cc:	shm@...ulusnetworks.com
Subject: Re: [PATCH net-next] vrf: Add ip rules at vrf device create


On 12/8/15 4:54 AM, Nikolay Aleksandrov wrote:
> Hi,
> On 12/08/2015 04:55 AM, David Ahern wrote:
> [snip]
>>
>> +static inline size_t vrf_fib_rule_nl_size(bool have_pref)
>> +{
>> +	size_t sz;
>> +
>> +	sz = NLMSG_ALIGN(sizeof(struct fib_rule_hdr))
>> +			 + nla_total_size(IFNAMSIZ) /* FRA_{I,O}IFNAME */
>> +			 + nla_total_size(4); /* FRA_TABLE, u32 */
> directly use sizeof(u32) and remove it from the comment ?

sure.

>
>> +
>> +	if (have_pref)
>> +		sz += nla_total_size(4); /* FRA_PRIORITY, u32 */
> Why not always add this to the size and remove the whole have_pref and sz ?

pref == 0 and pref not set are completely different configurations and 
the message should not contain extra bytes.

>
>> +
>> +	return sz;
>> +}
>> +
>> +static int vrf_fib_rule(struct net_device *dev, __u8 family, int if_type,
>> +			bool add_it)
> I think dev can be constified.
>
>> +{
>> +	struct net_vrf *vrf = netdev_priv(dev);
> vrf is only read and can be const
>
>> +	struct fib_rule_hdr *frh;
>> +	struct nlmsghdr *nlh;
>> +	struct sk_buff *skb;
>> +	int err;
>> +
>> +	skb = nlmsg_new(vrf_fib_rule_nl_size(!!vrf->pref), GFP_KERNEL);
>> +	if (!skb)
>> +		return -ENOMEM;
>> +
>> +	nlh = nlmsg_put(skb, 0, 0, 0, sizeof(*frh), 0);
>> +	if (!nlh)
>> +		return -EMSGSIZE;
> Looks like the skb will be leaked here because nlmsg_put() doesn't free it.
> In fact I can see this error in other places throughout the stack (probably
> c&p errors), good that it's not supposed to happen. :-)
>
>> +
>> +	frh = nlmsg_data(nlh);
>> +	memset(frh, 0, sizeof(*frh));
>> +	frh->family = family;
>> +	frh->action = FR_ACT_TO_TBL;
>> +
>> +	if (nla_put_u32(skb, FRA_TABLE, vrf->tb_id))
>> +		goto nla_put_failure;
>> +
>> +	if (nla_put_string(skb, if_type, dev->name))
>> +		goto nla_put_failure;
>> +
>> +	if (vrf->pref) {
>> +		if (nla_put_u32(skb, FRA_PRIORITY, vrf->pref))
>> +			goto nla_put_failure;
>> +	}
>> +
>> +	nlmsg_end(skb, nlh);
>> +
>> +	/* fib_nl_{new,del}rule handling looks for net from skb->sk */
>> +	skb->sk = dev_net(dev)->rtnl;
>> +	if (add_it) {
>> +		err = fib_nl_newrule(skb, nlh);
>> +	} else {
>> +		err = fib_nl_delrule(skb, nlh);
>> +		if (err == -ENOENT)
>> +			err = 0;
>> +	}
>> +
>> +	kfree_skb(skb);
> minor nit: instead of kfree_skb(), you can use nlmsg_free() which currently
> does the same, but would be clearer that this is the nlmsg skb.
>
>> +
>> +	return err;
>> +
>> +nla_put_failure:
>> +	nlmsg_cancel(skb, nlh);
>
> Here you'll leak the skb, nlmsg_cancel() only trims it and removes
> the message, but doesn't free the skb.
>
>> +	return -EMSGSIZE;
>> +}
>> +
>> +static void vrf_del_fib_rules(struct net_device *dev)
> Same here for dev (const)
>
>> +{
>> +	if (vrf_fib_rule(dev, AF_INET,  FRA_IIFNAME, 0) ||
>> +	    vrf_fib_rule(dev, AF_INET,  FRA_OIFNAME, 0) ||
>> +	    vrf_fib_rule(dev, AF_INET6, FRA_IIFNAME, 0) ||
>> +	    vrf_fib_rule(dev, AF_INET6, FRA_OIFNAME, 0)) {
>> +		netdev_err(dev, "Failed to delete FIB rules for %s\n",
>> +			   dev->name);
>
> I've seen this use of netdev_err() elsewhere in vrf, too. I was going to
> send a patch to change it because you get messages like:
> <dev name>: Failed to add FIB rules for <dev name>
> which is pointless. You can just drop the extra dev->name.
>
>> +	}
>> +}
>> +
>> +static int vrf_add_fib_rules(struct net_device *dev)
> Same here for dev (const)
>
>> +{
>> +	int err;
>> +
>> +	err = vrf_fib_rule(dev, AF_INET,  FRA_IIFNAME, 1);
>> +	if (err < 0)
>> +		goto out_err;
>> +
>> +	err = vrf_fib_rule(dev, AF_INET,  FRA_OIFNAME, 1);
>> +	if (err < 0)
>> +		goto out_err;
>> +
>> +	err = vrf_fib_rule(dev, AF_INET6, FRA_IIFNAME, 1);
>> +	if (err < 0)
>> +		goto out_err;
>> +
>> +	err = vrf_fib_rule(dev, AF_INET6, FRA_OIFNAME, 1);
>> +	if (err < 0)
>> +		goto out_err;
>> +
>> +	return 0;
>> +out_err:
>> +	netdev_err(dev, "Failed to add FIB rules for %s\n", dev->name);
> Same here for dev->name
>
>> +	vrf_del_fib_rules(dev);
>> +	return err;
>> +}
>> +
> [snip]
>

ack on all the rest. Thanks for the review, Nik.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html