Re: [PATCH bpf-next v2 1/7] netkit, bpf: Add bpf programmable net device

From: Andrew Lunn <andrew@lunn.ch>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: bpf@vger.kernel.org, netdev@vger.kernel.org,
	martin.lau@linux.dev, razor@blackwall.org, ast@kernel.org,
	andrii@kernel.org, john.fastabend@gmail.com, sdf@google.com,
	toke@kernel.org
Subject: Re: [PATCH bpf-next v2 1/7] netkit, bpf: Add bpf programmable net device
Date: Sat, 21 Oct 2023 00:18:53 +0200	[thread overview]
Message-ID: <33467f55-4bbf-4078-af21-d91c6aab82ee@lunn.ch> (raw)
In-Reply-To: <20231019204919.4203-2-daniel@iogearbox.net>

> +static void netkit_get_drvinfo(struct net_device *dev,
> +			       struct ethtool_drvinfo *info)
> +{
> +	strscpy(info->driver, DRV_NAME, sizeof(info->driver));
> +	strscpy(info->version, "n/a", sizeof(info->version));

If you don't put anything in version, the core will put in the git
hash of the kernel. Its more useful than "n/a".

> +	ether_setup(dev);
> +	dev->min_mtu = ETH_MIN_MTU;

ether_setup() sets min_mtu to ETH_MIN_MTU.

> +static int netkit_new_link(struct net *src_net, struct net_device *dev,
> +			   struct nlattr *tb[], struct nlattr *data[],
> +			   struct netlink_ext_ack *extack)
> +{

...

> +	err = register_netdevice(peer);
> +	put_net(net);
> +	if (err < 0)
> +		goto err_register_peer;
> +
> +	netif_carrier_off(peer);
> +
> +	err = rtnl_configure_link(peer, ifmp, 0, NULL);
> +	if (err < 0)
> +		goto err_configure_peer;

Seeing code after calling register_netdevice() often means bugs. The
interface is live, and in use before the function even returns. The
kernel can try to get an IP address, mount an NFS root etc. This might
be safe, because you have two linked interfaces here, and the other
one is not yet registered. Maybe some comment about this would be
good, or can the rtnl_configure_link() be done earlier?

> +
> +	if (mode == NETKIT_L2)
> +		eth_hw_addr_random(dev);
> +	if (tb[IFLA_IFNAME])
> +		nla_strscpy(dev->name, tb[IFLA_IFNAME], IFNAMSIZ);
> +	else
> +		snprintf(dev->name, IFNAMSIZ, "m%%d");
> +
> +	err = register_netdevice(dev);
> +	if (err < 0)
> +		goto err_configure_peer;

We have the same here, but now we have both peers registers, the
kernel could of configured both up in order to find its NFS root etc.
Is it safe to have packets flowing at this point? Before the remaining
configuration happens?

> +
> +	netif_carrier_off(dev);
> +
> +	nk = netdev_priv(dev);
> +	nk->primary = true;
> +	nk->policy = default_prim;
> +	nk->mode = mode;
> +	if (nk->mode == NETKIT_L2)
> +		dev_change_flags(dev, dev->flags & ~IFF_NOARP, NULL);
> +	bpf_mprog_bundle_init(&nk->bundle);
> +	RCU_INIT_POINTER(nk->active, NULL);
> +	rcu_assign_pointer(nk->peer, peer);
> +
> +	nk = netdev_priv(peer);
> +	nk->primary = false;
> +	nk->policy = default_peer;
> +	nk->mode = mode;
> +	if (nk->mode == NETKIT_L2)
> +		dev_change_flags(peer, peer->flags & ~IFF_NOARP, NULL);
> +	bpf_mprog_bundle_init(&nk->bundle);
> +	RCU_INIT_POINTER(nk->active, NULL);
> +	rcu_assign_pointer(nk->peer, dev);
> +	return 0;
> +err_configure_peer:
> +	unregister_netdevice(peer);
> +	return err;
> +err_register_peer:
> +	free_netdev(peer);
> +	return err;
> +}

  Andrew