From: Jiri Pirko <jiri@resnulli.us>
To: Nikolay Aleksandrov <razor@blackwall.org>
Cc: "Daniel Borkmann" <daniel@iogearbox.net>,
bpf@vger.kernel.org, netdev@vger.kernel.org,
martin.lau@linux.dev, ast@kernel.org, andrii@kernel.org,
john.fastabend@gmail.com, sdf@google.com, toke@kernel.org,
kuba@kernel.org, andrew@lunn.ch,
"Toke Høiland-Jørgensen" <toke@redhat.com>
Subject: Re: [PATCH bpf-next v4 1/7] netkit, bpf: Add bpf programmable net device
Date: Thu, 26 Oct 2023 07:26:08 +0200 [thread overview]
Message-ID: <ZTn4cOgrCOZAvIUO@nanopsycho> (raw)
In-Reply-To: <5df35b1b-0a63-a73f-7a32-c6c87f4676cc@blackwall.org>
Wed, Oct 25, 2023 at 09:21:01PM CEST, razor@blackwall.org wrote:
>On 10/25/23 18:47, Jiri Pirko wrote:
>> Tue, Oct 24, 2023 at 11:48:58PM CEST, daniel@iogearbox.net wrote:
>> > This work adds a new, minimal BPF-programmable device called "netkit"
>[snip]
>>
>> Couple of nitpicks below:
>>
>> [..]
>>
>>
>
>Hi,
>Thanks for the review. I know about the nits below but decided against
>changing them, more below each...
>
>> > +static int netkit_check_policy(int policy, struct nlattr *tb,
>> > + struct netlink_ext_ack *extack)
>> > +{
>> > + switch (policy) {
>> > + case NETKIT_PASS:
>> > + case NETKIT_DROP:
>> > + return 0;
>> > + default:
>>
>> Isn't this job for netlink policy?
>>
>>
>
>This cannot be handled by policies AFAIK, because only 2 sparse values from
>more are allowed. We could potentially do it through validate() but
Perhaps good time to extend the netlink validation for list
of values allowed?
>it's the same minus the explicit policy type info. IMO this approach is good.
>
>> > + NL_SET_ERR_MSG_ATTR(extack, tb,
>> > + "Provided default xmit policy not supported");
>> > + return -EINVAL;
>> > + }
>> > +}
>> > +
>> > +static int netkit_check_mode(int mode, struct nlattr *tb,
>> > + struct netlink_ext_ack *extack)
>> > +{
>> > + switch (mode) {
>> > + case NETKIT_L2:
>> > + case NETKIT_L3:
>> > + return 0;
>> > + default:
>>
>> Isn't this job for netlink policy?
>>
>>
>
>This one can be handled by policy indeed, but then we lose the nice user
>error. Again can be done through validate(), but it's the same and we
>lose explicit policy type information.
But the netlink validator setups the extack properly. What's wrong with
that?
>
>> > + NL_SET_ERR_MSG_ATTR(extack, tb,
>> > + "Provided device mode can only be L2 or L3");
>> > + return -EINVAL;
>> > + }
>> > +}
>> > +
>> > +static int netkit_validate(struct nlattr *tb[], struct nlattr *data[],
>> > + struct netlink_ext_ack *extack)
>> > +{
>> > + struct nlattr *attr = tb[IFLA_ADDRESS];
>> > +
>> > + if (!attr)
>> > + return 0;
>> > + NL_SET_ERR_MSG_ATTR(extack, attr,
>> > + "Setting Ethernet address is not supported");
>> > + return -EOPNOTSUPP;
>> > +}
>> > +
>> > +static struct rtnl_link_ops netkit_link_ops;
>> > +
>> > +static int netkit_new_link(struct net *src_net, struct net_device *dev,
>> > + struct nlattr *tb[], struct nlattr *data[],
>> > + struct netlink_ext_ack *extack)
>> > +{
>> > + struct nlattr *peer_tb[IFLA_MAX + 1], **tbp = tb, *attr;
>> > + enum netkit_action default_prim = NETKIT_PASS;
>> > + enum netkit_action default_peer = NETKIT_PASS;
>> > + enum netkit_mode mode = NETKIT_L3;
>> > + unsigned char ifname_assign_type;
>> > + struct ifinfomsg *ifmp = NULL;
>> > + struct net_device *peer;
>> > + char ifname[IFNAMSIZ];
>> > + struct netkit *nk;
>> > + struct net *net;
>> > + int err;
>> > +
>> > + if (data) {
>> > + if (data[IFLA_NETKIT_MODE]) {
>> > + attr = data[IFLA_NETKIT_MODE];
>> > + mode = nla_get_u32(attr);
>> > + err = netkit_check_mode(mode, attr, extack);
>> > + if (err < 0)
>> > + return err;
>> > + }
>> > + if (data[IFLA_NETKIT_PEER_INFO]) {
>> > + attr = data[IFLA_NETKIT_PEER_INFO];
>> > + ifmp = nla_data(attr);
>> > + err = rtnl_nla_parse_ifinfomsg(peer_tb, attr, extack);
>> > + if (err < 0)
>> > + return err;
>> > + err = netkit_validate(peer_tb, NULL, extack);
>> > + if (err < 0)
>> > + return err;
>> > + tbp = peer_tb;
>> > + }
>> > + if (data[IFLA_NETKIT_POLICY]) {
>> > + attr = data[IFLA_NETKIT_POLICY];
>> > + default_prim = nla_get_u32(attr);
>> > + err = netkit_check_policy(default_prim, attr, extack);
>> > + if (err < 0)
>> > + return err;
>> > + }
>> > + if (data[IFLA_NETKIT_PEER_POLICY]) {
>> > + attr = data[IFLA_NETKIT_PEER_POLICY];
>> > + default_peer = nla_get_u32(attr);
>> > + err = netkit_check_policy(default_peer, attr, extack);
>> > + if (err < 0)
>> > + return err;
>> > + }
>> > + }
>> > +
>> > + if (ifmp && tbp[IFLA_IFNAME]) {
>> > + nla_strscpy(ifname, tbp[IFLA_IFNAME], IFNAMSIZ);
>> > + ifname_assign_type = NET_NAME_USER;
>> > + } else {
>> > + strscpy(ifname, "nk%d", IFNAMSIZ);
>> > + ifname_assign_type = NET_NAME_ENUM;
>> > + }
>> > +
>> > + net = rtnl_link_get_net(src_net, tbp);
>> > + if (IS_ERR(net))
>> > + return PTR_ERR(net);
>> > +
>> > + peer = rtnl_create_link(net, ifname, ifname_assign_type,
>> > + &netkit_link_ops, tbp, extack);
>> > + if (IS_ERR(peer)) {
>> > + put_net(net);
>> > + return PTR_ERR(peer);
>> > + }
>> > +
>> > + netif_inherit_tso_max(peer, dev);
>> > +
>> > + if (mode == NETKIT_L2)
>> > + eth_hw_addr_random(peer);
>> > + if (ifmp && dev->ifindex)
>> > + peer->ifindex = ifmp->ifi_index;
>> > +
>> > + nk = netkit_priv(peer);
>> > + nk->primary = false;
>> > + nk->policy = default_peer;
>> > + nk->mode = mode;
>> > + bpf_mprog_bundle_init(&nk->bundle);
>> > + RCU_INIT_POINTER(nk->active, NULL);
>> > + RCU_INIT_POINTER(nk->peer, NULL);
>>
>> Aren't these already 0?
>>
>>
>
>Yep, they are. Here decided in favor of explicit show of values, although
>it's minor and I'm fine either way.
It is always confusing to see this. Reader might think this is necessary
as if the mem was not previuosly cleared. The general approach is to
rely on mem being zeroed, not sure why this is different.
>
>> > +
>> > + err = register_netdevice(peer);
>> > + put_net(net);
>> > + if (err < 0)
>> > + goto err_register_peer;
>> > + netif_carrier_off(peer);
>> > + if (mode == NETKIT_L2)
>> > + dev_change_flags(peer, peer->flags & ~IFF_NOARP, NULL);
>> > +
>> > + err = rtnl_configure_link(peer, NULL, 0, NULL);
>> > + if (err < 0)
>> > + goto err_configure_peer;
>> > +
>> > + if (mode == NETKIT_L2)
>> > + eth_hw_addr_random(dev);
>> > + if (tb[IFLA_IFNAME])
>> > + nla_strscpy(dev->name, tb[IFLA_IFNAME], IFNAMSIZ);
>> > + else
>> > + strscpy(dev->name, "nk%d", IFNAMSIZ);
>> > +
>> > + nk = netkit_priv(dev);
>> > + nk->primary = true;
>> > + nk->policy = default_prim;
>> > + nk->mode = mode;
>> > + bpf_mprog_bundle_init(&nk->bundle);
>> > + RCU_INIT_POINTER(nk->active, NULL);
>> > + RCU_INIT_POINTER(nk->peer, NULL);
>> > +
>> > + err = register_netdevice(dev);
>> > + if (err < 0)
>> > + goto err_configure_peer;
>> > + netif_carrier_off(dev);
>> > + if (mode == NETKIT_L2)
>> > + dev_change_flags(dev, dev->flags & ~IFF_NOARP, NULL);
>> > +
>> > + rcu_assign_pointer(netkit_priv(dev)->peer, peer);
>> > + rcu_assign_pointer(netkit_priv(peer)->peer, dev);
>> > + return 0;
>> > +err_configure_peer:
>> > + unregister_netdevice(peer);
>> > + return err;
>> > +err_register_peer:
>> > + free_netdev(peer);
>> > + return err;
>> > +}
>> > +
>>
>> [..]
>
>Cheers,
> Nik
>
next prev parent reply other threads:[~2023-10-26 5:26 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-24 21:48 [PATCH bpf-next v4 0/7] Add bpf programmable net device Daniel Borkmann
2023-10-24 21:48 ` [PATCH bpf-next v4 1/7] netkit, bpf: " Daniel Borkmann
2023-10-25 15:47 ` Jiri Pirko
2023-10-25 17:20 ` Daniel Borkmann
2023-10-26 5:18 ` Jiri Pirko
2023-10-26 12:11 ` Daniel Borkmann
2023-10-25 19:21 ` Nikolay Aleksandrov
2023-10-26 5:26 ` Jiri Pirko [this message]
2023-10-26 6:21 ` Nikolay Aleksandrov
2023-10-25 21:24 ` Kui-Feng Lee
2023-10-25 22:09 ` Martin KaFai Lau
2023-10-26 1:15 ` Kui-Feng Lee
2023-10-26 1:18 ` Kui-Feng Lee
2023-10-26 6:20 ` Daniel Borkmann
2023-10-26 17:47 ` Kui-Feng Lee
2023-10-26 18:46 ` Martin KaFai Lau
2023-10-24 21:48 ` [PATCH bpf-next v4 2/7] tools: Sync if_link uapi header Daniel Borkmann
2023-10-24 21:49 ` [PATCH bpf-next v4 3/7] libbpf: Add link-based API for netkit Daniel Borkmann
2023-10-24 21:49 ` [PATCH bpf-next v4 4/7] bpftool: Implement link show support " Daniel Borkmann
2023-10-24 21:49 ` [PATCH bpf-next v4 5/7] bpftool: Extend net dump with netkit progs Daniel Borkmann
2023-10-24 21:49 ` [PATCH bpf-next v4 6/7] selftests/bpf: Add netlink helper library Daniel Borkmann
2023-10-24 21:49 ` [PATCH bpf-next v4 7/7] selftests/bpf: Add selftests for netkit Daniel Borkmann
2023-10-24 22:45 ` [PATCH bpf-next v4 0/7] Add bpf programmable net device Martin KaFai Lau
2023-10-24 23:50 ` patchwork-bot+netdevbpf
2023-10-25 15:50 ` Jiri Pirko
2023-10-25 16:54 ` Martin KaFai Lau
2023-10-26 5:35 ` Jiri Pirko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZTn4cOgrCOZAvIUO@nanopsycho \
--to=jiri@resnulli.us \
--cc=andrew@lunn.ch \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=john.fastabend@gmail.com \
--cc=kuba@kernel.org \
--cc=martin.lau@linux.dev \
--cc=netdev@vger.kernel.org \
--cc=razor@blackwall.org \
--cc=sdf@google.com \
--cc=toke@kernel.org \
--cc=toke@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).