On Thu, May 28, 2020 at 8:45 AM Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
>
> - me->personality &= ~bprm->per_clear;
> + if (bprm->per_clear)
> + me->personality &= ~PER_CLEAR_ON_SETID;\
My only problem with this patch is that I find that 'per_clear' thing
to be a horrid horrid name,
Obviously the name didn't change, but the use *did* change, and as
such the name got worse. It used do do things like
bprm->per_clear |= PER_CLEAR_ON_SETID;
and now it does
bprm->per_clear = 1;
and honestly, there's a lot more semantic context in the old code that
is now missing entirely. At least you used to be able to grep for
PER_CLEAR_ON_SETID and it would make you go "Ahh.."
Put another way, I can kind of see what a line like
bprm->per_clear |= PER_CLEAR_ON_SETID;
does, simply because now it kind of hints at what is up.
But what the heck does
bprm->per_clear = 1;
mean? Nothing. You have to really know the code. "per_clear" makes no
sense, and now it's a short line that doesn't need to be that short.
I think "bprm->clear_personality_bits" would maybe describe what the
_effect_ of that field is. It doesn't explain _why_, but it at least
explains "what" much better than "per_clear", which just makes me go
"per what?".
Alternatively, "bprm->creds_changed" would describe what the bit
conceptually is about, and code like
if (bprm->creds_changed)
me->personality &= ~PER_CLEAR_ON_SETID;\
looks sensible to me and kind of matches the comment about the
PER_CLEAR_ON_SETID bits are.
So I think that using a bitfield is fine, but I'd really like it to be
named something much better.
Plus changing the name means that you can't have any code that now
mistakenly uses the new semantics but expects the old bitmask.
Generally when something changes semantics that radically, you want to
make sure the type changes sufficiently that any out-of-tree patch
that hasn't been merged yet will get a clear warning or error if
people don't realize.
Please?
Linus
