Re: [bpf PATCH v4 1/3] net: add a UID to use for ULP socket assignment

[Daniel Borkmann <daniel@iogearbox.net>]

On 02/05/2018 07:17 PM, John Fastabend wrote:
[...]
> @@ -124,6 +145,34 @@ int tcp_set_ulp(struct sock *sk, const char *name)
>  	if (!ulp_ops)
>  		return -ENOENT;
>  
> +	if (!ulp_ops->user_visible) {
> +		module_put(ulp_ops->owner);
> +		return -ENOENT;
> +	}
> +
> +	err = ulp_ops->init(sk);
> +	if (err) {
> +		module_put(ulp_ops->owner);
> +		return err;
> +	}
> +
> +	icsk->icsk_ulp_ops = ulp_ops;
> +	return 0;
> +}
> +
> +int tcp_set_ulp_id(struct sock *sk, int ulp)
> +{
> +	struct inet_connection_sock *icsk = inet_csk(sk);
> +	const struct tcp_ulp_ops *ulp_ops;
> +	int err;
> +
> +	if (icsk->icsk_ulp_ops)
> +		return -EEXIST;
> +
> +	ulp_ops = __tcp_ulp_lookup(ulp);
> +	if (!ulp_ops)
> +		return -ENOENT;
> +
>  	err = ulp_ops->init(sk);
>  	if (err) {

Just some minor feedback; v5 would have been impractical for just this
as the fix looks otherwise fine, but I thought I'd mention it here for a
possible follow-up cleanup to address:

tcp_set_ulp() and tcp_set_ulp_id() are quite different from API PoV despite
that they almost look the same: tcp_set_ulp() respects user_visible bool
while tcp_set_ulp_id() ignores it.

I'm wondering, could we somehow make this more obvious from the API itself?

Perhaps:

  int tcp_set_ulp_id(struct sock *sk, int ulp, bool user_visible_only)

And we do:

	[...]
	if (user_visible_only && !ulp_ops->user_visible) {
		module_put(ulp_ops->owner);
		return -ENOENT;
	}

Maybe something like this would make it more obvious for callers, potentially
same could be added in tcp_set_ulp(), so both APIs only differ in name vs id
but not much more (yeah, one pulls in the module as well, but aside from that).
Then, they could also be consolidated, since the only thing different left
is __tcp_ulp_find_autoload() vs __tcp_ulp_lookup() to fetch the actual ulp_ops.
Wdyt?

Thanks,
Daniel