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!
