netdev - Re: [PATCH net-next 5/6] sfc: neighbour lookup for TC encap action offload

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 7 Jun 2023 10:34:16 +0200
From: Simon Horman <simon.horman@...igine.com>
To: edward.cree@....com
Cc: linux-net-drivers@....com, davem@...emloft.net, kuba@...nel.org,
	pabeni@...hat.com, edumazet@...gle.com,
	Edward Cree <ecree.xilinx@...il.com>, netdev@...r.kernel.org,
	habetsm.xilinx@...il.com
Subject: Re: [PATCH net-next 5/6] sfc: neighbour lookup for TC encap action
 offload

On Mon, Jun 05, 2023 at 08:17:38PM +0100, edward.cree@....com wrote:

...

> +static int efx_bind_neigh(struct efx_nic *efx,
> +			  struct efx_tc_encap_action *encap, struct net *net,
> +			  struct netlink_ext_ack *extack)
> +{
> +	struct efx_neigh_binder *neigh, *old;
> +	struct flowi6 flow6 = {};
> +	struct flowi4 flow4 = {};
> +	int rc;
> +
> +	/* GCC stupidly thinks that only values explicitly listed in the enum
> +	 * definition can _possibly_ be sensible case values, so without this
> +	 * cast it complains about the IPv6 versions.
> +	 */
> +	switch ((int)encap->type) {
> +	case EFX_ENCAP_TYPE_VXLAN:
> +	case EFX_ENCAP_TYPE_GENEVE:
> +		flow4.flowi4_proto = IPPROTO_UDP;
> +		flow4.fl4_dport = encap->key.tp_dst;
> +		flow4.flowi4_tos = encap->key.tos;
> +		flow4.daddr = encap->key.u.ipv4.dst;
> +		flow4.saddr = encap->key.u.ipv4.src;
> +		break;
> +	case EFX_ENCAP_TYPE_VXLAN | EFX_ENCAP_FLAG_IPV6:
> +	case EFX_ENCAP_TYPE_GENEVE | EFX_ENCAP_FLAG_IPV6:
> +		flow6.flowi6_proto = IPPROTO_UDP;
> +		flow6.fl6_dport = encap->key.tp_dst;
> +		flow6.flowlabel = ip6_make_flowinfo(encap->key.tos,
> +						    encap->key.label);
> +		flow6.daddr = encap->key.u.ipv6.dst;
> +		flow6.saddr = encap->key.u.ipv6.src;
> +		break;
> +	default:
> +		NL_SET_ERR_MSG_FMT_MOD(extack, "Unsupported encap type %d",
> +				       (int)encap->type);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	neigh = kzalloc(sizeof(*neigh), GFP_KERNEL_ACCOUNT);
> +	if (!neigh)
> +		return -ENOMEM;
> +	neigh->net = get_net(net);
> +	neigh->dst_ip = flow4.daddr;
> +	neigh->dst_ip6 = flow6.daddr;
> +
> +	old = rhashtable_lookup_get_insert_fast(&efx->tc->neigh_ht,
> +						&neigh->linkage,
> +						efx_neigh_ht_params);
> +	if (old) {
> +		/* don't need our new entry */
> +		put_net(neigh->net);
> +		kfree(neigh);
> +		if (!refcount_inc_not_zero(&old->ref))
> +			return -EAGAIN;
> +		/* existing entry found, ref taken */
> +		neigh = old;
> +	} else {
> +		/* New entry.  We need to initiate a lookup */
> +		struct neighbour *n;
> +		struct rtable *rt;
> +
> +		if (encap->type & EFX_ENCAP_FLAG_IPV6) {
> +#if IS_ENABLED(CONFIG_IPV6)
> +			struct dst_entry *dst;
> +
> +			dst = ipv6_stub->ipv6_dst_lookup_flow(net, NULL, &flow6,
> +							      NULL);
> +			rc = PTR_ERR_OR_ZERO(dst);
> +			if (rc) {
> +				NL_SET_ERR_MSG_MOD(extack, "Failed to lookup route for IPv6 encap");
> +				goto out_free;
> +			}
> +			dev_hold(neigh->egdev = dst->dev);
> +			neigh->ttl = ip6_dst_hoplimit(dst);
> +			n = dst_neigh_lookup(dst, &flow6.daddr);
> +			dst_release(dst);
> +#else
> +			/* We shouldn't ever get here, because if IPv6 isn't
> +			 * enabled how did someone create an IPv6 tunnel_key?
> +			 */
> +			rc = -EOPNOTSUPP;
> +			NL_SET_ERR_MSG_MOD(extack, "No IPv6 support (neigh bind)");
> +#endif
> +		} else {
> +			rt = ip_route_output_key(net, &flow4);
> +			if (IS_ERR_OR_NULL(rt)) {
> +				rc = PTR_ERR(rt);

Hi Edward,

A minor nit from my side: perhaps this should use PTR_ERR_OR_ZERO().

> +				if (!rc)
> +					rc = -EIO;
> +				NL_SET_ERR_MSG_MOD(extack, "Failed to lookup route for encap");
> +				goto out_free;
> +			}
> +			dev_hold(neigh->egdev = rt->dst.dev);
> +			neigh->ttl = ip4_dst_hoplimit(&rt->dst);
> +			n = dst_neigh_lookup(&rt->dst, &flow4.daddr);
> +			ip_rt_put(rt);
> +		}
> +		if (!n) {
> +			rc = -ENETUNREACH;
> +			NL_SET_ERR_MSG_MOD(extack, "Failed to lookup neighbour for encap");
> +			dev_put(neigh->egdev);
> +			goto out_free;
> +		}
> +		refcount_set(&neigh->ref, 1);
> +		INIT_LIST_HEAD(&neigh->users);
> +		read_lock_bh(&n->lock);
> +		ether_addr_copy(neigh->ha, n->ha);
> +		neigh->n_valid = n->nud_state & NUD_VALID;
> +		read_unlock_bh(&n->lock);
> +		rwlock_init(&neigh->lock);
> +		INIT_WORK(&neigh->work, efx_neigh_update);
> +		neigh->efx = efx;
> +		neigh->used = jiffies;
> +		if (!neigh->n_valid)
> +			/* Prod ARP to find us a neighbour */
> +			neigh_event_send(n, NULL);
> +		neigh_release(n);
> +	}
> +	/* Add us to this neigh */
> +	encap->neigh = neigh;
> +	list_add_tail(&encap->list, &neigh->users);
> +	return 0;
> +
> +out_free:
> +	/* cleanup common to several error paths */
> +	rhashtable_remove_fast(&efx->tc->neigh_ht, &neigh->linkage,
> +			       efx_neigh_ht_params);
> +	synchronize_rcu();
> +	put_net(net);
> +	kfree(neigh);
> +	return rc;
> +}

...