| Message ID | CAJ-EccOQxDjSgUL0AsCywoKDbOUNWDyxCKHQc+s6+ZemUh9Uzw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL] SafeSetID changes for v5.10 | expand |
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
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
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
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
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
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!
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
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(-)