| Message ID | 20240304150914.11444-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, Mar 04, 2024 at 04:08:56PM +0100, Antonio Quartulli wrote: > Allow userspace to create and destroy an interface using netlink > commands. > > Signed-off-by: Antonio Quartulli <antonio@openvpn.net> > --- > drivers/net/ovpn/netlink.c | 50 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c > index 2e855ce145e7..02b41034f615 100644 > --- a/drivers/net/ovpn/netlink.c > +++ b/drivers/net/ovpn/netlink.c > @@ -154,7 +154,57 @@ static void ovpn_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb > dev_put(ovpn->dev); > } > > +static int ovpn_nl_new_iface(struct sk_buff *skb, struct genl_info *info) > +{ > + enum ovpn_mode mode = OVPN_MODE_P2P; > + struct net_device *dev; > + char *ifname; > + int ret; > + > + if (!info->attrs[OVPN_A_IFNAME]) > + return -EINVAL; > + > + ifname = nla_data(info->attrs[OVPN_A_IFNAME]); > + > + if (info->attrs[OVPN_A_MODE]) { > + mode = nla_get_u8(info->attrs[OVPN_A_MODE]); > + netdev_dbg(dev, "%s: setting device (%s) mode: %u\n", __func__, ifname, > + mode); nit: dev is uninitialised here. > + } > + > + ret = ovpn_iface_create(ifname, mode, genl_info_net(info)); > + if (ret < 0) > + netdev_dbg(dev, "error while creating interface %s: %d\n", ifname, ret); And here. Flagged by Smatch. > + > + return ret; > +} ...
On 05/03/2024 15:51, Simon Horman wrote: > On Mon, Mar 04, 2024 at 04:08:56PM +0100, Antonio Quartulli wrote: >> Allow userspace to create and destroy an interface using netlink >> commands. >> >> Signed-off-by: Antonio Quartulli <antonio@openvpn.net> >> --- >> drivers/net/ovpn/netlink.c | 50 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 50 insertions(+) >> >> diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c >> index 2e855ce145e7..02b41034f615 100644 >> --- a/drivers/net/ovpn/netlink.c >> +++ b/drivers/net/ovpn/netlink.c >> @@ -154,7 +154,57 @@ static void ovpn_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb >> dev_put(ovpn->dev); >> } >> >> +static int ovpn_nl_new_iface(struct sk_buff *skb, struct genl_info *info) >> +{ >> + enum ovpn_mode mode = OVPN_MODE_P2P; >> + struct net_device *dev; >> + char *ifname; >> + int ret; >> + >> + if (!info->attrs[OVPN_A_IFNAME]) >> + return -EINVAL; >> + >> + ifname = nla_data(info->attrs[OVPN_A_IFNAME]); >> + >> + if (info->attrs[OVPN_A_MODE]) { >> + mode = nla_get_u8(info->attrs[OVPN_A_MODE]); >> + netdev_dbg(dev, "%s: setting device (%s) mode: %u\n", __func__, ifname, >> + mode); > > nit: dev is uninitialised here. > >> + } >> + >> + ret = ovpn_iface_create(ifname, mode, genl_info_net(info)); >> + if (ret < 0) >> + netdev_dbg(dev, "error while creating interface %s: %d\n", ifname, ret); > > And here. > > Flagged by Smatch. Thanks for reporting these. Regards,
Antonio Quartulli <antonio@openvpn.net> writes: > Allow userspace to create and destroy an interface using netlink > commands. > > Signed-off-by: Antonio Quartulli <antonio@openvpn.net> > --- > drivers/net/ovpn/netlink.c | 50 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c > index 2e855ce145e7..02b41034f615 100644 > --- a/drivers/net/ovpn/netlink.c > +++ b/drivers/net/ovpn/netlink.c > @@ -154,7 +154,57 @@ static void ovpn_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb > dev_put(ovpn->dev); > } > > +static int ovpn_nl_new_iface(struct sk_buff *skb, struct genl_info *info) > +{ > + enum ovpn_mode mode = OVPN_MODE_P2P; > + struct net_device *dev; > + char *ifname; > + int ret; > + > + if (!info->attrs[OVPN_A_IFNAME]) > + return -EINVAL; > + > + ifname = nla_data(info->attrs[OVPN_A_IFNAME]); > + > + if (info->attrs[OVPN_A_MODE]) { > + mode = nla_get_u8(info->attrs[OVPN_A_MODE]); > + netdev_dbg(dev, "%s: setting device (%s) mode: %u\n", __func__, ifname, > + mode); Maybe print out the message even if the default mode is used, as the mode is applied in ovpn_iface_create anyways. /Esben
On 25/03/2024 16:01, Esben Haabendal wrote: > Antonio Quartulli <antonio@openvpn.net> writes: > >> Allow userspace to create and destroy an interface using netlink >> commands. >> >> Signed-off-by: Antonio Quartulli <antonio@openvpn.net> >> --- >> drivers/net/ovpn/netlink.c | 50 ++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 50 insertions(+) >> >> diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c >> index 2e855ce145e7..02b41034f615 100644 >> --- a/drivers/net/ovpn/netlink.c >> +++ b/drivers/net/ovpn/netlink.c >> @@ -154,7 +154,57 @@ static void ovpn_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb >> dev_put(ovpn->dev); >> } >> >> +static int ovpn_nl_new_iface(struct sk_buff *skb, struct genl_info *info) >> +{ >> + enum ovpn_mode mode = OVPN_MODE_P2P; >> + struct net_device *dev; >> + char *ifname; >> + int ret; >> + >> + if (!info->attrs[OVPN_A_IFNAME]) >> + return -EINVAL; >> + >> + ifname = nla_data(info->attrs[OVPN_A_IFNAME]); >> + >> + if (info->attrs[OVPN_A_MODE]) { >> + mode = nla_get_u8(info->attrs[OVPN_A_MODE]); >> + netdev_dbg(dev, "%s: setting device (%s) mode: %u\n", __func__, ifname, >> + mode); > > Maybe print out the message even if the default mode is used, as the > mode is applied in ovpn_iface_create anyways. Being this a debug message, my reasoning was "let's print what we got via netlink" (if nothing is printed, we know we are applying the default). Otherwise, when printing "P2P" we wouldn't be able to understand if it was set by default or received via netlink. Does it make sense? Cheers, > > /Esben
Antonio Quartulli <antonio@openvpn.net> writes: > On 25/03/2024 16:01, Esben Haabendal wrote: >> Antonio Quartulli <antonio@openvpn.net> writes: >> >>> Allow userspace to create and destroy an interface using netlink >>> commands. >>> >>> Signed-off-by: Antonio Quartulli <antonio@openvpn.net> >>> --- >>> drivers/net/ovpn/netlink.c | 50 ++++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 50 insertions(+) >>> >>> diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c >>> index 2e855ce145e7..02b41034f615 100644 >>> --- a/drivers/net/ovpn/netlink.c >>> +++ b/drivers/net/ovpn/netlink.c >>> @@ -154,7 +154,57 @@ static void ovpn_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb >>> dev_put(ovpn->dev); >>> } >>> +static int ovpn_nl_new_iface(struct sk_buff *skb, struct genl_info *info) >>> +{ >>> + enum ovpn_mode mode = OVPN_MODE_P2P; >>> + struct net_device *dev; >>> + char *ifname; >>> + int ret; >>> + >>> + if (!info->attrs[OVPN_A_IFNAME]) >>> + return -EINVAL; >>> + >>> + ifname = nla_data(info->attrs[OVPN_A_IFNAME]); >>> + >>> + if (info->attrs[OVPN_A_MODE]) { >>> + mode = nla_get_u8(info->attrs[OVPN_A_MODE]); >>> + netdev_dbg(dev, "%s: setting device (%s) mode: %u\n", __func__, ifname, >>> + mode); >> Maybe print out the message even if the default mode is used, as the >> mode is applied in ovpn_iface_create anyways. > > Being this a debug message, my reasoning was "let's print what we got via > netlink" (if nothing is printed, we know we are applying the default). > > Otherwise, when printing "P2P" we wouldn't be able to understand if it was set > by default or received via netlink. > > Does it make sense? Yes, reasoning is sane. And the prefixing with __func__ should help making it clear. /Esben
diff --git a/drivers/net/ovpn/netlink.c b/drivers/net/ovpn/netlink.c index 2e855ce145e7..02b41034f615 100644 --- a/drivers/net/ovpn/netlink.c +++ b/drivers/net/ovpn/netlink.c @@ -154,7 +154,57 @@ static void ovpn_post_doit(const struct genl_split_ops *ops, struct sk_buff *skb dev_put(ovpn->dev); } +static int ovpn_nl_new_iface(struct sk_buff *skb, struct genl_info *info) +{ + enum ovpn_mode mode = OVPN_MODE_P2P; + struct net_device *dev; + char *ifname; + int ret; + + if (!info->attrs[OVPN_A_IFNAME]) + return -EINVAL; + + ifname = nla_data(info->attrs[OVPN_A_IFNAME]); + + if (info->attrs[OVPN_A_MODE]) { + mode = nla_get_u8(info->attrs[OVPN_A_MODE]); + netdev_dbg(dev, "%s: setting device (%s) mode: %u\n", __func__, ifname, + mode); + } + + ret = ovpn_iface_create(ifname, mode, genl_info_net(info)); + if (ret < 0) + netdev_dbg(dev, "error while creating interface %s: %d\n", ifname, ret); + + return ret; +} + +static int ovpn_nl_del_iface(struct sk_buff *skb, struct genl_info *info) +{ + struct ovpn_struct *ovpn = info->user_ptr[0]; + + rtnl_lock(); + ovpn_iface_destruct(ovpn, true); + dev_put(ovpn->dev); + rtnl_unlock(); + + /* we set the user_ptr to NULL to prevent post_doit from releasing it again */ + info->user_ptr[0] = NULL; + + return 0; +} + static const struct genl_small_ops ovpn_nl_ops[] = { + { + .cmd = OVPN_CMD_NEW_IFACE, + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, + .doit = ovpn_nl_new_iface, + }, + { + .cmd = OVPN_CMD_DEL_IFACE, + .flags = GENL_ADMIN_PERM | GENL_CMD_CAP_DO, + .doit = ovpn_nl_del_iface, + }, }; static struct genl_family ovpn_nl_family __ro_after_init = {
Allow userspace to create and destroy an interface using netlink commands. Signed-off-by: Antonio Quartulli <antonio@openvpn.net> --- drivers/net/ovpn/netlink.c | 50 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)