netdev - Re: [PATCH net-next v3 05/24] ovpn: implement interface creation/destruction via netlink

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]

Date: Wed, 8 May 2024 11:49:07 +0200
From: Antonio Quartulli <antonio@...nvpn.net>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, Sergey Ryazanov <ryazanov.s.a@...il.com>,
 Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
 Andrew Lunn <andrew@...n.ch>, Esben Haabendal <esben@...nix.com>
Subject: Re: [PATCH net-next v3 05/24] ovpn: implement interface
 creation/destruction via netlink

On 08/05/2024 02:21, Jakub Kicinski wrote:
> On Mon,  6 May 2024 03:16:18 +0200 Antonio Quartulli wrote:
>>   int ovpn_nl_new_iface_doit(struct sk_buff *skb, struct genl_info *info)
>>   {
>> -	return -ENOTSUPP;
>> +	const char *ifname = OVPN_DEFAULT_IFNAME;
>> +	enum ovpn_mode mode = OVPN_MODE_P2P;
>> +	struct net_device *dev;
>> +	struct sk_buff *msg;
>> +	void *hdr;
>> +
>> +	if (info->attrs[OVPN_A_IFNAME])
>> +		ifname = nla_data(info->attrs[OVPN_A_IFNAME]);
>> +
>> +	if (info->attrs[OVPN_A_MODE]) {
>> +		mode = nla_get_u32(info->attrs[OVPN_A_MODE]);
>> +		pr_debug("ovpn: setting device (%s) mode: %u\n", ifname, mode);
>> +	}
>> +
>> +	dev = ovpn_iface_create(ifname, mode, genl_info_net(info));
>> +	if (IS_ERR(dev)) {
>> +		pr_err("ovpn: error while creating interface %s: %ld\n", ifname,
>> +		       PTR_ERR(dev));
> 
> Better to send the error to the caller with NL_SET_ERR_MSG_MOD()

yeah, makes sense. I guess I can do the same for every other error 
generated in any netlink handler.

> 
>> +		return PTR_ERR(dev);
>> +	}
>> +
>> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>> +	if (!msg)
>> +		return -ENOMEM;
>> +
>> +	hdr = genlmsg_put(msg, info->snd_portid, info->snd_seq, &ovpn_nl_family,
>> +			  0, OVPN_CMD_NEW_IFACE);
> 
> genlmsg_iput() will save you a lot of typing

oh, wow, nice one :) will switch to iput()

> 
>> +	if (!hdr) {
>> +		netdev_err(dev, "%s: cannot create message header\n", __func__);
>> +		return -EMSGSIZE;
>> +	}
>> +
>> +	if (nla_put(msg, OVPN_A_IFNAME, strlen(dev->name) + 1, dev->name)) {
> 
> nla_put_string() ?
> 

right.

>> +		netdev_err(dev, "%s: cannot add ifname to reply\n", __func__);
> 
> Probably not worth it, can't happen given the message size

Personally I still prefer to check the return value of functions that 
may fail, because somebody may break the assumption (i.e. message large 
enough by design) without realizing that this call was relying on that.

If you want, I could still add a comment saying that we don't expect 
this to happen.

> 
>> +		genlmsg_cancel(msg, hdr);
>> +		nlmsg_free(msg);
>> +		return -EMSGSIZE;
>> +	}
>> +
>> +	genlmsg_end(msg, hdr);
>> +
>> +	return genlmsg_reply(msg, info);
>>   }
>>   
>>   int ovpn_nl_del_iface_doit(struct sk_buff *skb, struct genl_info *info)
>>   {
>> -	return -ENOTSUPP;
>> +	struct ovpn_struct *ovpn = info->user_ptr[0];
>> +
>> +	rtnl_lock();
>> +	ovpn_iface_destruct(ovpn);
>> +	dev_put(ovpn->dev);
>> +	rtnl_unlock();
>> +
>> +	synchronize_net();
> 
> Why? 🤔️


hmm I was under the impression that we should always call this function 
when destroying an interface to make sure that packets that already 
entered the network stack can be properly processed before the interface 
is gone for good.

Maybe this is not the right place? Any hint?

-- 
Antonio Quartulli
OpenVPN Inc.