| Message ID | 20240506011637.27272-6-antonio@openvpn.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introducing OpenVPN Data Channel Offload | expand |
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() > + 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 > + 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() ? > + netdev_err(dev, "%s: cannot add ifname to reply\n", __func__); Probably not worth it, can't happen given the message size > + 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?
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?
On Wed, 8 May 2024 11:49:07 +0200 Antonio Quartulli wrote: > >> + 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. In a few other places we put a WARN_ON_ONCE() on messages size errors. That way syzbot usually catches the miscalculation rather quickly. But no strong objections if you prefer the print. > >> + 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?
On 09/05/2024 03:09, Jakub Kicinski wrote: > On Wed, 8 May 2024 11:49:07 +0200 Antonio Quartulli wrote: >>>> + 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. > > In a few other places we put a WARN_ON_ONCE() on messages size errors. > That way syzbot usually catches the miscalculation rather quickly. > But no strong objections if you prefer the print. I am fine as long as we have some check. If WARN_ON_ONCE() helps syzbot, then I'll go with it. > >>>> + 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?
diff --git a/drivers/net/ovpn/main.h b/drivers/net/ovpn/main.h index 21d6bfb27d67..12b8d7e4a0fe 100644 --- a/drivers/net/ovpn/main.h +++ b/drivers/net/ovpn/main.h @@ -10,6 +10,8 @@ #ifndef _NET_OVPN_MAIN_H_ #define _NET_OVPN_MAIN_H_ +#define OVPN_DEFAULT_IFNAME "ovpn%d" + /** * ovpn_iface_create - create and initialize a new 'ovpn' netdevice * @name: the name of the new device diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c index c0a9f58e0e87..66f5c6fbe8e4 100644 --- a/drivers/net/ovpn/netlink.c +++ b/drivers/net/ovpn/netlink.c @@ -7,6 +7,7 @@ */ #include <linux/netdevice.h> +#include <linux/rtnetlink.h> #include <net/genetlink.h> #include <uapi/linux/ovpn.h> @@ -78,12 +79,62 @@ void ovpn_nl_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb, 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)); + 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); + 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)) { + netdev_err(dev, "%s: cannot add ifname to reply\n", __func__); + 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(); + + return 0; } int ovpn_nl_set_peer_doit(struct sk_buff *skb, struct genl_info *info)
Allow userspace to create and destroy an interface using netlink commands. Signed-off-by: Antonio Quartulli <antonio@openvpn.net> --- drivers/net/ovpn/main.h | 2 ++ drivers/net/ovpn/netlink.c | 55 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 55 insertions(+), 2 deletions(-)