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

Antonio Quartulli
2024-05-08T09:47:54Z

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.