[GIT PULL] SafeSetID changes for v5.10

[GIT PULL] SafeSetID changes for v5.10

[Micah Morton]

The following changes since commit bbf5c979011a099af5dc76498918ed7df445635b:

  Linux 5.9 (2020-10-11 14:15:50 -0700)

are available in the Git repository at:

  https://github.com/micah-morton/linux.git tags/safesetid-5.10

for you to fetch changes up to 03ca0ec138927b16fab0dad7b869f42eb2849c94:

  LSM: SafeSetID: Fix warnings reported by test bot (2020-10-13 09:17:36 -0700)

----------------------------------------------------------------
SafeSetID changes for v5.10

The changes in this pull request are mostly contained to within the
SafeSetID LSM, with the exception of a few 1-line changes to change
some ns_capable() calls to ns_capable_setid() -- causing a flag
(CAP_OPT_INSETID) to be set that is examined by SafeSetID code and
nothing else in the kernel. These changes have been baking in -next and
actually were in -next for the entire v5.9 merge window but I didn't
have a chance to send them.

The changes to SafeSetID internally allow for setting up GID transition
security policies, as already existed for UIDs.

NOTE: I'm re-using my safesetid-next branch here as the branch for
creating the pull request. I think that's fine, not sure if this is the
normal workflow or not. Also, I use 'git rebase vX.X' to put my commits
on top of the latest stable release. Again, I verified with gitk that I
don't have any weird history in my branch that will mess things up so
AFAICT that should be fine too.

----------------------------------------------------------------

Thomas Cedeno (3):
      LSM: Signal to SafeSetID when setting group IDs
      LSM: SafeSetID: Add GID security policy handling
      LSM: SafeSetID: Fix warnings reported by test bot

 Documentation/admin-guide/LSM/SafeSetID.rst |  29 +++--
 kernel/capability.c                         |   2 +-
 kernel/groups.c                             |   2 +-
 kernel/sys.c                                |  10 +-
 security/safesetid/lsm.c                    | 190 +++++++++++++++++++++-------
 security/safesetid/lsm.h                    |  38 ++++--
 security/safesetid/securityfs.c             | 190 ++++++++++++++++++++--------
 7 files changed, 336 insertions(+), 125 deletions(-)

Re: [GIT PULL] SafeSetID changes for v5.10

[Linus Torvalds]

These were rebased since the merge window started, for no apparent reason.

Were they in linux-next?

And if so, why was I sent some different version?

             Linus

Re: [GIT PULL] SafeSetID changes for v5.10

[Micah Morton]

On Thu, Oct 15, 2020 at 4:06 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> These were rebased since the merge window started, for no apparent reason.
>
> Were they in linux-next?

Yeah, they are changes that were originally targeting the v5.9 merge
window (and thus were in -next during July/August) but I didn't get
the chance to send a pull request for them. Since I didn't touch my
-next branch since then they are also in 'next-20201013' and
'next-20201015'.

I just rebased to v5.9 to make sure the 1-line changes that touch
kernel/capability.c, kernel/groups.c and kernel/sys.c still applied
cleanly without conflicts. Should I have rebased onto one of the -rc's
for v5.9 instead?

>
> And if so, why was I sent some different version?
>
>              Linus

Re: [GIT PULL] SafeSetID changes for v5.10

[Linus Torvalds]

On Thu, Oct 15, 2020 at 5:01 PM Micah Morton <mortonm@chromium.org> wrote:
>
> I just rebased to v5.9 to make sure the 1-line changes that touch
> kernel/capability.c, kernel/groups.c and kernel/sys.c still applied
> cleanly without conflicts. Should I have rebased onto one of the -rc's
> for v5.9 instead?

No. You shouldn't have rebased at all.

Making sure something applies cleanly is simply not a reason to rebase.

See

  Documentation/maintainer/rebasing-and-merging.rst

for some common rules.

             Linus

Re: [GIT PULL] SafeSetID changes for v5.10

[Micah Morton]

On Fri, Oct 23, 2020 at 10:57 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Thu, Oct 15, 2020 at 5:01 PM Micah Morton <mortonm@chromium.org> wrote:
> >
> > I just rebased to v5.9 to make sure the 1-line changes that touch
> > kernel/capability.c, kernel/groups.c and kernel/sys.c still applied
> > cleanly without conflicts. Should I have rebased onto one of the -rc's
> > for v5.9 instead?
>
> No. You shouldn't have rebased at all.

Ok so before the rebase ("reparent"), the commits were based on top of
some commit that was months old at this point (can't quite remember
now, I think one of the -rc's for v5.8).

I guess the lesson here is never rebase or fast-forward merge my
upstream-bound -next branch until it has emptied entirely into the
mainline? And if that doesn't happen for whatever reason during one
merge window and I have to wait for the next one, I just send you
un-reparented/un-fastforwarded possibly outdated commits and you will
resolve conflicts if any?

The reason for the rebase making sense to me here was that the changes
to common kernel code are very simple (a few one line changes) and
easy to quickly verify after the rebase -- and the vast majority of
the complexity of the code in the pull request was confined to the
SafeSetID code base, which had no changes over the time span from
original base to the reparented base. So I had basically considered it
a no-op rebase. I probably should have explained this in the pull
request.

Thanks

>
> Making sure something applies cleanly is simply not a reason to rebase.
>
> See
>
>   Documentation/maintainer/rebasing-and-merging.rst
>
> for some common rules.
>
>              Linus

Re: [GIT PULL] SafeSetID changes for v5.10

[Linus Torvalds]

On Fri, Oct 23, 2020 at 12:15 PM Micah Morton <mortonm@chromium.org> wrote:
>
> Ok so before the rebase ("reparent"), the commits were based on top of
> some commit that was months old at this point (can't quite remember
> now, I think one of the -rc's for v5.8).

Nobody cares if the old parent is old. In fact, that's usually a good
sign that the code has had testing and is changing things that aren't
in flux for other reasons.

It's often a good idea to make a test-merge and verify that things are
ok, but that's for your _personal_ verification, and shouldn't be
something that anybody else sees.

And even with a test-merge, it doesn't matter if there is some simple
conflict - we have those all the time, and conflicts aren't bad. In
fact, they allow me to see "ok, things have changed here in parallel",
and I'll be aware of it.

The main reason to rebase is if things have changed _so_ much that you
really need to re-do things, or if there is some major bug in _your_
branch that simply needs to be fixed.

>    So I had basically considered it
> a no-op rebase. I probably should have explained this in the pull
> request.

If it's a no-op rebase, thern DON'T DO IT. Really. It just means that
now you have lost all the testing.

Thinking that it's a no-op doesn't really help. No bugs are
_intentional_, I would seriously hope. Lack of testing is lack of
testing, regardless of whether you think it would not matter.

It also destroys the real history of the code, which is sad.

Now, sometimes you may _want_ to destroy the real history of the code
(as in "Oh, this history is too ugly to survive, and makes bisection
impossible because some of the intermediate state was seriously
buggy"). That is then one of those few valid reasons to rebase (see
the "major bug in your branch" case above).

But 99% of the time, rebasing is bad. If it was in linux-next and
there were no horrible problems with it, and it got tested there, then
just leave it alone and don't destroy the testing it did get.

Anyway, I've pulled this now, but honestly, don't do this again. Stop
rebasing without a big and immediate reason, and stop destroying
whatever testing it got in linux-next.

And if you _do_ rebase, and you _do_ have a real and very serious
reason, then mention that reason and explain it. But no "the rebase
didn't make any difference" isn't a reason. Quite the reverse.

               Linus

                   Linus

Re: [GIT PULL] SafeSetID changes for v5.10

[pr-tracker-bot]

The pull request you sent on Thu, 15 Oct 2020 11:15:18 -0700:

> https://github.com/micah-morton/linux.git tags/safesetid-5.10

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/81ecf91eab1045c009b5d73408c44033ba86bb4d

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

Re: [GIT PULL] SafeSetID changes for v5.10

[Micah Morton]

On Sun, Oct 25, 2020 at 10:54 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, Oct 23, 2020 at 12:15 PM Micah Morton <mortonm@chromium.org> wrote:
> >
> > Ok so before the rebase ("reparent"), the commits were based on top of
> > some commit that was months old at this point (can't quite remember
> > now, I think one of the -rc's for v5.8).
>
> Nobody cares if the old parent is old. In fact, that's usually a good
> sign that the code has had testing and is changing things that aren't
> in flux for other reasons.

Ok thanks for the explanation, I think this was the most important
piece I was missing, but makes sense now.

>
> It's often a good idea to make a test-merge and verify that things are
> ok, but that's for your _personal_ verification, and shouldn't be
> something that anybody else sees.
>
> And even with a test-merge, it doesn't matter if there is some simple
> conflict - we have those all the time, and conflicts aren't bad. In
> fact, they allow me to see "ok, things have changed here in parallel",
> and I'll be aware of it.
>
> The main reason to rebase is if things have changed _so_ much that you
> really need to re-do things, or if there is some major bug in _your_
> branch that simply needs to be fixed.

Yeah sounds good, I'll just get in the habit of doing a test merge and
note in the pull request whether there are any conflicts or not.

>
> >    So I had basically considered it
> > a no-op rebase. I probably should have explained this in the pull
> > request.
>
> If it's a no-op rebase, thern DON'T DO IT. Really. It just means that
> now you have lost all the testing.
>
> Thinking that it's a no-op doesn't really help. No bugs are
> _intentional_, I would seriously hope. Lack of testing is lack of
> testing, regardless of whether you think it would not matter.
>
> It also destroys the real history of the code, which is sad.
>
> Now, sometimes you may _want_ to destroy the real history of the code
> (as in "Oh, this history is too ugly to survive, and makes bisection
> impossible because some of the intermediate state was seriously
> buggy"). That is then one of those few valid reasons to rebase (see
> the "major bug in your branch" case above).
>
> But 99% of the time, rebasing is bad. If it was in linux-next and
> there were no horrible problems with it, and it got tested there, then
> just leave it alone and don't destroy the testing it did get.
>
> Anyway, I've pulled this now, but honestly, don't do this again. Stop
> rebasing without a big and immediate reason, and stop destroying
> whatever testing it got in linux-next.

Got it.

>
> And if you _do_ rebase, and you _do_ have a real and very serious
> reason, then mention that reason and explain it. But no "the rebase
> didn't make any difference" isn't a reason. Quite the reverse.
>
>                Linus
>
>                    Linus