On 08/05/2024 02:21, Jakub Kicinski wrote:

On Mon,  6 May 2024 03:16:18 +0200 Antonio Quartulli wrote:

quoted

  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.

quoted

+		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()

quoted

+	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.

quoted

+		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.

quoted

+		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.