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.
>
> 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?*
- 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.
