On Wed, Jul 22, 2026 at 11:59 AM Georgia Garcia <[email protected]> wrote: > > On Wed, 2026-07-22 at 11:30 -0700, Ryan Lee wrote: > > On Wed, Jul 22, 2026 at 8:22 AM Georgia Garcia > > <[email protected]> wrote: > > > > > > Hey, > > > > > > I don't think this is correct. At least I don't see the leakage. > > > > This commit is not fixing a leak, but is instead making a previous fix > > for a refcount leak more efficient. > > > > > > My apologies, I misunderstood the intent of the patch. Could you maybe > state in the commit message that this is an optimization? > > > > > > > On Mon, 2026-07-06 at 14:36 -0700, Ryan Lee via AppArmor wrote: > > > > Commit > > > > 6d25e7b47616cb2db43351210929c8f19dc305a3 ("apparmor: fix refcount leak > > > > when updating the sk_ctx") > > > > > > > > fixes a refcount leak by unconditionally getting and putting a reference > > > > to plabel in update_sk_ctx. However, update_sk_ctx can instead inform > > > > its > > > > caller (aa_unix_file_perm) whether or not it persists a reference, and > > > > then conditionally put the reference if it is not used. Semantically, > > > > this > > > > involves update_sk_ctx now conditionally transferring the refcount, and > > > > informing aa_unix_file_perm whether the refcount was used (and thus > > > > whether it should be put as part of cleanups). > > > > > > > > Signed-off-by: Ryan Lee <[email protected]> > > > > --- > > > > security/apparmor/af_unix.c | 30 +++++++++++++++++++++--------- > > > > 1 file changed, 21 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/security/apparmor/af_unix.c b/security/apparmor/af_unix.c > > > > index 9ca9d1b890ba..2730f4ad08d3 100644 > > > > --- a/security/apparmor/af_unix.c > > > > +++ b/security/apparmor/af_unix.c > > > > @@ -643,13 +643,16 @@ int aa_unix_peer_perm(const struct cred > > > > *subj_cred, > > > > peer_label); > > > > } > > > > > > > > -/* sk_plabel for comparison only */ > > > > -static void update_sk_ctx(struct sock *sk, struct aa_label *label, > > > > +/* sk_plabel for comparison only > > > > + * Returns whether plabel was assigned to a pointer > > > > + */ > > > > +static bool update_sk_ctx(struct sock *sk, struct aa_label *label, > > > > struct aa_label *plabel) > > > > { > > > > struct aa_label *l, *old; > > > > struct aa_sk_ctx *ctx = aa_sock(sk); > > > > bool update_sk; > > > > + bool plabel_used = false; > > > > > > > > rcu_read_lock(); > > > > update_sk = (plabel && > > > > @@ -658,7 +661,7 @@ static void update_sk_ctx(struct sock *sk, struct > > > > aa_label *label, > > > > !__aa_subj_label_is_cached(label, rcu_dereference(ctx->label)); > > > > rcu_read_unlock(); > > > > if (!update_sk) > > > > - return; > > > > + return false; > > > > > > > > spin_lock(&unix_sk(sk)->lock); > > > > old = rcu_dereference_protected(ctx->label, > > > > @@ -675,16 +678,17 @@ static void update_sk_ctx(struct sock *sk, struct > > > > aa_label *label, > > > > old = rcu_dereference_protected(ctx->peer, > > > > lockdep_is_held(&unix_sk(sk)->lock)); > > > > > > > > if (old == plabel) { > > > > - rcu_assign_pointer(ctx->peer_lastupdate, > > > > - aa_get_label(plabel)); > > > > + rcu_assign_pointer(ctx->peer_lastupdate, plabel); > > > > + plabel_used = true; > > > > } else if (aa_label_is_subset(plabel, old)) { > > > > - rcu_assign_pointer(ctx->peer_lastupdate, > > > > - aa_get_label(plabel)); > > > > + rcu_assign_pointer(ctx->peer_lastupdate, plabel); > > > > > > In apparmor_sk_free_security we put the label on both peer and > > > peer_lastupdate, so this would cause 2 puts for 1 get, which isn't > > > right. > > > > > > > rcu_assign_pointer(ctx->peer, > > > > aa_get_label(plabel)); > > > > aa_put_label(old); > > > > + plabel_used = true; > > > > } /* else race or a subset - don't update */ > > > > } > > > > spin_unlock(&unix_sk(sk)->lock); > > > > + return plabel_used; > > > > } > > > > > > > > static void update_peer_ctx(struct sock *sk, struct aa_sk_ctx *ctx, > > > > @@ -721,6 +725,7 @@ int aa_unix_file_perm(const struct cred *subj_cred, > > > > struct aa_label *label, > > > > struct path path; > > > > bool is_sk_fs; > > > > int error = 0; > > > > + bool plabel_owner_transfer = false; > > > > > > > > AA_BUG(!label); > > > > AA_BUG(!sock); > > > > @@ -799,8 +804,15 @@ int aa_unix_file_perm(const struct cred > > > > *subj_cred, struct aa_label *label, > > > > > > > > /* update peer cache to latest successful perm check */ > > > > if (error == 0) > > > > - update_sk_ctx(sock->sk, label, plabel); > > > > - aa_put_label(plabel); > > > > > > I don't think the put above was meant to address the "get" done in > > > update_sk_ctx, I believe it was meant for the plabel = > > > aa_get_newest_label(pctx->label); > > > > This was also my understanding: that this aa_put_label call is meant > > to pair with the aa_get_newest_label call earlier in this function. > > > > > > > > And the gets done in update_sk_ctx would be addressed by > > > apparmor_sk_free_security. > > > > I have a further explanation at the bottom of this email explaining > > how I traced the refcount changes. > > > > > > > > > > > > + plabel_owner_transfer = update_sk_ctx(sock->sk, label, > > > > plabel); > > > > + > > > > + /* If plabel ownership was not transferred, plabel can be either > > > > null > > > > + * (we never got a ref) or non-null (we got a ref and nobody else > > > > will > > > > + * use it, so we need to put it). Either way, aa_put_label will do > > > > + * the right thing. > > > > + */ > > > > + if (!plabel_owner_transfer) > > > > + aa_put_label(plabel); > > > > > > > > return error; > > > > } > > > > > > > To sketch out how I view the refcounts in the control flow of > > aa_unix_file_perm (all this assuming plabel is not null): > > > > Before this patch: > > > > plabel is obtained as aa_get_newest_label(pctx->label) (+1) > > unix_peer_perm and __aa_subj_label_is_cached leave the plabel refcount > > as they were before > > if (error != 0), plabel is not used again and is put, leaving the > > refcount balanced (+1 -1 = 0) > > if (error == 0), update_sk_ctx is called: > > - if ctx->peer_lastupdate != plabel: > > -- compare ctx->peer to plabel. If same: > > --- update only ctx->peer_lastupdate, taking a new refcount to plabel > > (+1 +1 = 2) > > -- else: > > --- update both ctx->peer_lastupdate and ctx->peer, taking new > > refcounts for each (+1 +1 +1 = 3) > > -- *Another potential issue spotted: do we need to put the refcount > > for whatever used to be in ctx->peer_lastupdate?* > > Yes, we should be put the refcount when peer_lastupdate is replaced, > good catch. > > > - After update_sk_ctx, plabel in aa_unix_file_perm gets put, with > > final refcount change being (+1 +1 -1 = 1) or (+1 +1 +1 -1 = 2) > > apparmor_sk_free_security frees the refcounts taken in update_sk_ctx, > > bringing the net refcount change back to 0 > > > > After this patch: > > > > plabel is obtained as aa_get_newest_label(pctx->label) (+1) > > unix_peer_perm and __aa_subj_label_is_cached leave the plabel refcount > > as they were before > > if (error != 0), plabel is not used again and is put, leaving the > > refcount balanced (+1 -1 = 0) > > if (error == 0), update_sk_ctx is called: > > - if ctx->peer_lastupdate != plabel: > > -- compare ctx->peer to plabel. If same: > > --- update only ctx->peer_lastupdate, reusing the existing refcount to > > plabel (+1 +0 = 1) > > -- else: > > --- update both ctx->peer_lastupdate and ctx->peer, reusing the > > existing refcount and taking only one new reference (+1 +0 +1 = 2) > > -- *This patch does not address the potential issue newly identified.* > > - After update_sk_ctx, plabel in aa_unix_file_perm gets conditionally > > put depending on whether the existing refcount was reused, with the > > final refcount change being (+1 +0 -0 = 1) or (+1 +0 +1 -0 = 2) > > apparmor_sk_free_security frees the refcounts taken in update_sk_ctx, > > bringing the net refcount change back to 0 > > > > Thus, the advantage of this patch is that it removes one get/put pair, > > reducing the number of atomic operations needed on the refcount > > member. > > > > Please let me know if you think I messed up in my analysis somewhere. > > Thank you for the detailed explanation, that makes sense!
I have sent a v2 to the mailing list as a 2-patch patch series, with the second patch fixing the ctx->peer_lastupdate issue as well.
