Re: [PATCH net 1/1] ipv6: flowlabel: enforce the per-socket lease cap on reused labels

[Jakub Kicinski <kuba@kernel.org>]

Please trim the hash in the Fixes tag.

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ipv6: flowlabel: enforce the per-socket lease cap on reused labels

This adds mem_check_dup() and calls it from the recheck/reuse path in
ipv6_flowlabel_get(), so that re-acquiring an already existing flowlabel
also has to pass the FL_MAX_PER_SOCK per-socket lease check. The
linger/expires refresh of the existing label is made conditional on that
check passing.

A few questions on the changelog and on the new check below.

[Severity: Low]
The changelog says:

  "mem_check() only counts socket leases after fl_size falls below
   FL_MAX_SIZE - FL_MAX_PER_SOCK. Reusing an existing flowlabel does not
   increase fl_size, so duplicate GET requests can keep taking the
   recheck path and grow one socket's lease list without ever hitting the
   FL_MAX_PER_SOCK limit."

Is that description of the mechanism accurate?  Two things look inverted.

First, the early return in mem_check() is:

	room = FL_MAX_SIZE - fl_size;

	if (room > FL_MAX_SIZE - FL_MAX_PER_SOCK)
		return 0;

which is room > 8160, i.e. fl_size < FL_MAX_PER_SOCK (32).  So the lease
count is skipped while fl_size is small, not "after fl_size falls below
FL_MAX_SIZE - FL_MAX_PER_SOCK" (8160), which is true nearly all of the
time.  Was room confused with fl_size here?

Second, before this patch the recheck block in ipv6_flowlabel_get() had no
mem_check() call at all, so the per-socket cap was not consulted on the
reuse path regardless of fl_size.  Should the message describe this as an
unconditional absence of accounting on the reuse path rather than a narrow
fl_size window?  That distinction matters for the stable tag, since the
reproducer is just: create a label with share != IPV6_FL_S_EXCL, then
repeatedly call setsockopt(IPV6_FLOWLABEL_MGR, IPV6_FL_A_GET) on it.

> diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
> index 1ab5ad0dcf24f..e295f85bcc604 100644
> --- a/net/ipv6/ip6_flowlabel.c
> +++ b/net/ipv6/ip6_flowlabel.c
> @@ -493,6 +493,24 @@ static int mem_check(struct sock *sk)
>  	return 0;
>  }
>  
> +static int mem_check_dup(struct sock *sk)
> +{
> +	struct ipv6_fl_socklist *sfl;
> +	int count = 0;
> +
> +	lockdep_assert_held(&ip6_fl_lock);
> +
> +	rcu_read_lock();
> +	for_each_sk_fl_rcu(sk, sfl)
> +		count++;
> +	rcu_read_unlock();

[Severity: Low]
Is ip6_fl_lock the right lock to assert here?  The only data
mem_check_dup() reads is the per-socket list inet_sk(sk)->ipv6_fl_list, and
this file documents that list as being guarded by ip6_sk_fl_lock:

	#define socklist_dereference(__sflp) \
		rcu_dereference_protected(__sflp, lockdep_is_held(&ip6_sk_fl_lock))

and fl_link() inserts under it:

	spin_lock_bh(&ip6_sk_fl_lock);
	sfl->fl = fl;
	sfl->next = inet->ipv6_fl_list;
	rcu_assign_pointer(inet->ipv6_fl_list, sfl);
	spin_unlock_bh(&ip6_sk_fl_lock);

ip6_fl_lock is described as:

	/* FL hash table lock: it protects only of GC */

and guards fl_ht[], fl_size and net->ipv6.flowlabel_count, none of which
mem_check_dup() touches (unlike mem_check(), which reads fl_size).  So the
assertion advertises a contract that is unrelated to the data being
walked, and the lock that actually serializes insertion and removal is not
held across the count and the following fl_link().

As far as I can tell there is no actual race today, because
ipv6_flowlabel_get() and ipv6_flowlabel_put() both run under
sockopt_lock_sock(sk) taken in do_ipv6_setsockopt() before
IPV6_FLOWLABEL_MGR is dispatched, and fl6_free_socklist() only runs at
teardown.  Would it be clearer to drop the assertion (and the new
spin_lock_bh(&ip6_fl_lock) around the call) and instead note the socket
lock invariant in a comment, or assert ip6_sk_fl_lock if a lock is wanted?

As written, the global ip6_fl_lock is held across an O(n) list walk that
needs none of the state it protects, which contends with
ip6_fl_gc()/fl_intern()/fl_release().

> +
> +	if (count >= FL_MAX_PER_SOCK && !capable(CAP_NET_ADMIN))
> +		return -ENOBUFS;
> +
> +	return 0;
> +}
> +

[Severity: Medium]
The count here is over all leases the socket holds, not over duplicates of
the requested label, and the check omits mem_check()'s room/fl_size and
net->ipv6.flowlabel_count gating.  That makes the reuse path strictly
stricter than the create path.  Is that intended?

Concretely, while fl_size < FL_MAX_PER_SOCK, mem_check() returns 0 early:

	room = FL_MAX_SIZE - fl_size;

	if (room > FL_MAX_SIZE - FL_MAX_PER_SOCK)
		return 0;

so the same unprivileged socket may still intern brand-new labels past 32
leases, while mem_check_dup() refuses to share an existing one at the same
count.  For example: create label L, take 31 further leases of L so
count == 32 with fl_size == 1, then IPV6_FL_F_CREATE a new label — room is
8191 > 8160 and mem_check() allows it, but a plain IPV6_FL_A_GET of an
existing label returns -ENOBUFS.

Should mem_check_dup() count only leases of the requested label, or reuse
the same gating that mem_check() applies?

>  static inline void fl_link(struct sock *sk, struct ipv6_fl_socklist *sfl,
>  			   struct ip6_flowlabel *fl)
>  {
> @@ -679,10 +697,17 @@ static int ipv6_flowlabel_get(struct sock *sk, struct in6_flowlabel_req *freq,
>  			err = -ENOMEM;
>  			if (!sfl1)
>  				goto release;
> -			if (fl->linger > fl1->linger)
> -				fl1->linger = fl->linger;
> -			if ((long)(fl->expires - fl1->expires) > 0)
> -				fl1->expires = fl->expires;
> +			spin_lock_bh(&ip6_fl_lock);
> +			err = mem_check_dup(sk);
> +			if (err == 0) {
> +				if (fl->linger > fl1->linger)
> +					fl1->linger = fl->linger;
> +				if ((long)(fl->expires - fl1->expires) > 0)
> +					fl1->expires = fl->expires;
> +			}
> +			spin_unlock_bh(&ip6_fl_lock);
> +			if (err != 0)
> +				goto release;

[Severity: Medium]
This block changes user-visible behaviour in ways the changelog does not
mention, since it says only that it keeps "the duplicate-acquisition path
consistent with the long-standing per-socket cap".

An IPV6_FL_A_GET that re-acquires or shares an existing label, which
previously always succeeded, now returns -ENOBUFS once the socket holds 32
leases of any labels.

When the cap fires, the fl1->linger / fl1->expires refresh is skipped, so
a GET-based refresh loop stops refreshing with no error path that
distinguishes it from a full failure.  Is IPV6_FL_A_RENEW meant to be the
only remaining way to refresh here?  Would it be better to keep the
refresh unconditional and only reject the new fl_link()?

The create branch is also affected.  After:

	err = mem_check(sk);
	if (err == 0)
		fl1 = fl_intern(net, fl, freq->flr_label);
	...
	if (fl1)
		goto recheck;

a plain IPV6_FL_A_GET|IPV6_FL_F_CREATE that merely loses a create race
against another socket re-enters this block and is re-adjudicated by the
stricter rule, so it can now fail with -ENOBUFS even though mem_check()
already approved it under ip6_fl_lock.  Is that acceptable?

Leases are only dropped one at a time by an explicit IPV6_FL_A_PUT in
ipv6_flowlabel_put(), or at teardown in fl6_free_socklist(), so a socket
that reaches the cap stays capped until it puts labels back.

>  			fl_link(sk, sfl1, fl1);
>  			fl_free(fl);
>  			return 0;
-- 
pw-bot: cr