Re: [GIT PULL] Audit patches for 4.5

[Paul Moore]

On Thursday, January 14, 2016 11:28:47 AM Stephen Rothwell wrote:
> Hi Paul,

Hello.
 
> On Wed, 13 Jan 2016 11:24:29 -0500 Paul Moore <pmoore@xxxxxxxxxx> wrote:
> > In December I made some changes to how I manage the SELinux and 
> > audit trees:
> >  * https://www.redhat.com/archives/linux-audit/2015-December/msg00019.html
> 
> You may have a problem here, you start with:
> 
> "1. When a new kernel is released, rebase the repository's upstream branch
> to the tagged kernel release (or the latest LSM upstream branch in the case
> of SELinux) and apply the next branch on top of the upstream branch.  Send
> a pull request for the upstream branch to the next level maintainer."
> 
> Linus has repeatedly said to not rebase just before sending a pull
> request unless you hava a good reason - and even then you should let
> the result be tested for a few days before sending a pull request.

Yes, I understand that, trust me I do; I've had exchanges before with Linus on 
this very topic.  I did try following this model for a few release with the 
audit tree (several more when you include the SELinux tree) and from a 
practical point of view it just doesn't work for the trees I'm responsible for 
managing.  Related changes happening upstream, outside the tree, and other 
cross tree efforts meant that we were rebasing at least every other kernel 
release, if not every kernel release to ensure proper behavior and sane 
merges.  Things were worse with the SELinux tree as James would typically 
reject pull requests that didn't pull/merge cleanly.  I'm sure Linus' words 
work for a lot of trees out there, but it didn't work very well for the audit 
or SELinux trees, and I'm speaking as someone who did try to make it work.

As for testing, the whole reason I made the changes I did were to help 
increase testing.  Based on the bugs we've seen over the years, I'm fairly 
confident that not much SELinux/audit testing happens during the development 
(linux-next) and -rcX stages; the current SELinux/audit tree process gives us 
pre-built kernel packages that are tested weekly ... and by more than just me 
(thank goodness).  Also, if you look how the kernel packages are generated 
(see the COPR link I sent previous for the details/scripts), you'll see that 
the SELinux/audit next branches are applied on top of the current -rcX 
release, allowing us to incrementally test a kernel that is *very* close to 
what we are going to see in the merge window.

> He also says that published trees (that people could be developing
> against) should not be rebased (except for exceptional circumstances).

Experience has shown that this is largely not an issue for either the SELinux 
or audit trees as this has proven to not be an issue for the limited number of 
developers involved.  What has proven to be an issue is changes made to the 
SELinux/audit code, or affecting the SELinux/audit code, in other trees which 
we can only resolve if we rebase.

I'm also very forgiving when it comes to merge conflicts in other developer's 
code as a result of the rebases; no one has yet to complain.

> Instead you could do
> 
> 1. When a new kernel is released (i.e. the merge window opens), send a
> pull request for the upstream branch to the next level maintainer.
> After it is merged, then do a fast forward bask merge merge of the
> upstream tree (if necessary).

You lost me on the "fast forward bask merge merge" - no worries, I often think 
faster than I type too ;)

> > ... I will readily admit it isn't a perfect system, in fact it is a
> > step back in some areas, but the changes make it easier for me to get
> > pre-built kernel packages to users who are interested in testing the
> > bleeding edge (the Fedora COPR repository, see below) and it helps me
> > keep up with weekly testing of both the -rcX kernel releases and the
> > changes in the SELinux and audit trees. One of the things I've been
> > trying to work on lately is better, more automated, testing of the
> > SELinux and audit bits in the Linux kernel; unfortunately, some
> > things have had to change a little to help make this happen, but I
> > think the more frequent testing outweighs any disadvantages.
> 
> I don't understand why this testing would require any rebasing.  You
> can just crate a test branch that is Linus' tree and then merge in your
> tree and test that.

Please see my comments above about cross tree changes as well as the COPR 
scripts.  If you have an alternate that works, I'm open to suggestions; just 
please don't say "only rebase when there is a good reason", as the need seems 
to happen often for the audit/SELinux trees.  Maybe I'm cursed, I dunno.

> > The date change is likely a result of moving the patches from
> > audit#next to audit#upstream as part of the process mentioned above.
> 
> I wasn't aware that "git rebase" would change the author dates by
> default (in fact I don;t think it does).  Or do you use some other
> method to move the patches.

Here is what I typically do (or something very similar) using a combination of 
git/stgit:

 NOTE: done in the upstream branch

 # git reset --hard <version tag>
 # stg pick -b next $(stg series -B next --noprefix)

... this isn't a rebase, this resets the upstream to the given kernel version 
tag and then moves/applies/whatever the patches from the next branch on top.  
It is intentionally done this way as this is effectively what I've been 
testing throughout the -rcX cycle ... the patches applied *on*top*.

> In any case, why aren't you just submitting the next branch upstream?

The number one reason is that James tends to reject the pull request if there 
is any merge conflicts.  I try to keep the same process for both the audit and 
SELinux trees; granted this isn't a strict requirement, but it does help my 
sanity.  It also doesn't truly reflect what/how we've been testing during the 
-rcX/development cycle (see my other comments).

> > I haven't updated audit#next yet because I know you try to keep
> > linux-next quiet until -rc1 is released; if that has changed let me
> > know and I'll be happy to update audit#next.
> 
> It hasn't changed, but this is part of what I tell everyone who adds a
> branch to linux-next:
> 
> "Basically, this should be just what you would send to Linus (or ask him
> to fetch)."

As I stated in my last email, there are some unfortunate side-effects of the 
current process, this is one of them.  I believe the increased testing and 
availability far outweighs the negative in this particular case.

> So by "quiet" I mean not adding stuff for the next release and not
> changing stuff around too much.  If you *must* rebase you tree for some
> reason, you should let it simmer in linux-next for a few days before
> asking Linus to pull it.  That way, at least Linus and I will end up
> with the same *commits* and I (and others) won't have to cope with
> unnecessary conflicts caused by different versions of the same
> *patches* (or even just further changes to teh same files in other
> commits.
> 
> > For reference, the Fedora COPR repository can be found below, it was
> > announced back in November, but only to the relevant lists.  Anyone
> > is welcome to give the kernels a try (instructions are provided) and
> > report any problems they find.  I tend to push out an update at least
> > once a week to coincide with the new -rcX release, although the exact
> > day varies due to merge conflicts, build problems, etc.
> > 
> >  * https://copr.fedoraproject.org/coprs/pcmoore/kernel-secnext
> 
> So, I don't see why that requires you to rebase your tree.  That kernel
> source is separate from Linus' in any case (since I assume it contains
> all manner of not-yet-upstreamed or back ported patches).

-- 
paul moore
security @ redhat