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]
Message-ID: <3b9f43da-0269-4eba-a3c4-dcb635c0de75@openvpn.net>
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.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ