| 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
| ||
|
Message-ID: <20220921060123.1236276d@kernel.org>
Date: Wed, 21 Sep 2022 06:01:23 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>,
Ido Schimmel <idosch@...dia.com>,
Petr Machata <petrm@...dia.com>,
Florent Fourcot <florent.fourcot@...irst.fr>,
Nikolay Aleksandrov <razor@...ckwall.org>,
Guillaume Nault <gnault@...hat.com>
Subject: Re: [PATCH net-next] rtnetlink: Honour NLM_F_ECHO flag in
rtnl_{new, set}link
On Wed, 21 Sep 2022 11:07:21 +0800 Hangbin Liu wrote:
> Netlink messages are used for communicating between user and kernel space.
> When user space configures the kernel with netlink messages, it can set the
> NLM_F_ECHO flag to request the kernel to send the applied configuration back
> to the caller. This allows user space to retrieve configuration information
> that are filled by the kernel (either because these parameters can only be
> set by the kernel or because user space let the kernel choose a default
> value).
>
> This patch handles NLM_F_ECHO flag and send link info back after
> rtnl_{new, set}link.
>
> Suggested-by: Guillaume Nault <gnault@...hat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> ---
>
> In this patch I use rtnl_unicast to send the nlmsg directly. But we can
> also pass "struct nlmsghdr *nlh" to rtnl_newlink_create() and
> do_setlink(), then call rtnl_notify to send the nlmsg. I'm not sure
> which way is better, any comments?
>
> For iproute2 patch, please see
> https://patchwork.kernel.org/project/netdevbpf/patch/20220916033428.400131-2-liuhangbin@gmail.com/
I feel like the justification for the change is lacking.
I'm biased [and frankly it takes a lot of self-restraint for me not
to say how I _really_ feel about netlink msg flags ;)] but IMO the
message flags fall squarely into the "this is magic which was never
properly implemented" bucket.
What makes this flag better than just issuing a GET command form user
space?
The flag was never checked on input and is not implemented by 99% of
netlink families and commands.
I'd love to hear what others think. IMO we should declare a moratorium
on any use of netlink flags and fixed fields, push netlink towards
being a simple conduit for TLVs.
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 74864dc46a7e..b65bd9ed8b0d 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -2645,13 +2645,38 @@ static int do_set_proto_down(struct net_device *dev,
> return 0;
> }
>
> +static int rtnl_echo_link_info(struct net_device *dev, u32 pid, u32 seq,
> + u32 ext_filter_mask, int tgt_netnsid)
> +{
> + struct sk_buff *skb;
> + int err;
> +
> + skb = nlmsg_new(if_nlmsg_size(dev, ext_filter_mask), GFP_KERNEL);
> + if (!skb)
> + return -ENOBUFS;
> +
> + err = rtnl_fill_ifinfo(skb, dev, dev_net(dev), RTM_NEWLINK, pid, seq,
> + 0, 0, ext_filter_mask, 0, NULL, 0,
> + tgt_netnsid, GFP_KERNEL);
> + if (err < 0) {
> + /* -EMSGSIZE implies BUG in if_nlmsg_size */
> + WARN_ON(err == -EMSGSIZE);
> + kfree_skb(skb);
> + } else {
> + err = rtnl_unicast(skb, dev_net(dev), pid);
> + }
> +
> + return err;
> +}
> +
> #define DO_SETLINK_MODIFIED 0x01
> /* notify flag means notify + modified. */
> #define DO_SETLINK_NOTIFY 0x03
> static int do_setlink(const struct sk_buff *skb,
> struct net_device *dev, struct ifinfomsg *ifm,
> struct netlink_ext_ack *extack,
> - struct nlattr **tb, int status)
> + struct nlattr **tb, int status,
> + u16 nlmsg_flags, u32 nlmsg_seq)
> {
> const struct net_device_ops *ops = dev->netdev_ops;
> char ifname[IFNAMSIZ];
> @@ -3009,6 +3034,21 @@ static int do_setlink(const struct sk_buff *skb,
> }
> }
>
> + if (nlmsg_flags & NLM_F_ECHO) {
> + u32 ext_filter_mask = 0;
> + int tgt_netnsid = -1;
> +
> + if (tb[IFLA_TARGET_NETNSID])
> + tgt_netnsid = nla_get_s32(tb[IFLA_TARGET_NETNSID]);
> +
> + if (tb[IFLA_EXT_MASK])
> + ext_filter_mask = nla_get_u32(tb[IFLA_EXT_MASK]);
> +
> + rtnl_echo_link_info(dev, NETLINK_CB(skb).portid,
> + nlmsg_seq, ext_filter_mask,
> + tgt_netnsid);
> + }
> +
> errout:
> if (status & DO_SETLINK_MODIFIED) {
> if ((status & DO_SETLINK_NOTIFY) == DO_SETLINK_NOTIFY)
> @@ -3069,7 +3109,9 @@ static int rtnl_setlink(struct sk_buff *skb, struct nlmsghdr *nlh,
> goto errout;
> }
>
> - err = do_setlink(skb, dev, ifm, extack, tb, 0);
> + err = do_setlink(skb, dev, ifm, extack, tb, 0,
> + nlh->nlmsg_flags, nlh->nlmsg_seq);
> +
> errout:
> return err;
> }
> @@ -3293,14 +3335,15 @@ static int rtnl_group_changelink(const struct sk_buff *skb,
> struct net *net, int group,
> struct ifinfomsg *ifm,
> struct netlink_ext_ack *extack,
> - struct nlattr **tb)
> + struct nlattr **tb, u16 nlmsg_flags, u32 nlmsg_seq)
> {
> struct net_device *dev, *aux;
> int err;
>
> for_each_netdev_safe(net, dev, aux) {
> if (dev->group == group) {
> - err = do_setlink(skb, dev, ifm, extack, tb, 0);
> + err = do_setlink(skb, dev, ifm, extack, tb, 0,
> + nlmsg_flags, nlmsg_seq);
> if (err < 0)
> return err;
> }
> @@ -3312,13 +3355,15 @@ static int rtnl_group_changelink(const struct sk_buff *skb,
> static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
> const struct rtnl_link_ops *ops,
> struct nlattr **tb, struct nlattr **data,
> - struct netlink_ext_ack *extack)
> + struct netlink_ext_ack *extack,
> + u16 nlmsg_flags, u32 nlmsg_seq)
> {
> unsigned char name_assign_type = NET_NAME_USER;
> struct net *net = sock_net(skb->sk);
> struct net *dest_net, *link_net;
> struct net_device *dev;
> char ifname[IFNAMSIZ];
> + int netnsid = -1;
> int err;
>
> if (!ops->alloc && !ops->setup)
> @@ -3336,9 +3381,9 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
> return PTR_ERR(dest_net);
>
> if (tb[IFLA_LINK_NETNSID]) {
> - int id = nla_get_s32(tb[IFLA_LINK_NETNSID]);
> + netnsid = nla_get_s32(tb[IFLA_LINK_NETNSID]);
>
> - link_net = get_net_ns_by_id(dest_net, id);
> + link_net = get_net_ns_by_id(dest_net, netnsid);
> if (!link_net) {
> NL_SET_ERR_MSG(extack, "Unknown network namespace id");
> err = -EINVAL;
> @@ -3382,6 +3427,17 @@ static int rtnl_newlink_create(struct sk_buff *skb, struct ifinfomsg *ifm,
> if (err)
> goto out_unregister;
> }
> +
> + if (nlmsg_flags & NLM_F_ECHO) {
> + u32 ext_filter_mask = 0;
> +
> + if (tb[IFLA_EXT_MASK])
> + ext_filter_mask = nla_get_u32(tb[IFLA_EXT_MASK]);
> +
> + rtnl_echo_link_info(dev, NETLINK_CB(skb).portid, nlmsg_seq,
> + ext_filter_mask, netnsid);
> + }
> +
> out:
> if (link_net)
> put_net(link_net);
> @@ -3544,7 +3600,8 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
> status |= DO_SETLINK_NOTIFY;
> }
>
> - return do_setlink(skb, dev, ifm, extack, tb, status);
> + return do_setlink(skb, dev, ifm, extack, tb, status,
> + nlh->nlmsg_flags, nlh->nlmsg_seq);
> }
>
> if (!(nlh->nlmsg_flags & NLM_F_CREATE)) {
> @@ -3556,7 +3613,8 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
> if (tb[IFLA_GROUP])
> return rtnl_group_changelink(skb, net,
> nla_get_u32(tb[IFLA_GROUP]),
> - ifm, extack, tb);
> + ifm, extack, tb,
> + nlh->nlmsg_flags, nlh->nlmsg_seq);
> return -ENODEV;
> }
>
> @@ -3578,7 +3636,8 @@ static int __rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
> return -EOPNOTSUPP;
> }
>
> - return rtnl_newlink_create(skb, ifm, ops, tb, data, extack);
> + return rtnl_newlink_create(skb, ifm, ops, tb, data, extack,
> + nlh->nlmsg_flags, nlh->nlmsg_seq);
> }
>
> static int rtnl_newlink(struct sk_buff *skb, struct nlmsghdr *nlh,
Powered by blists - more mailing lists