LWN: Comments on "A discussion on stable kernel workflow issues"

A discussion on stable kernel workflow issues

Wol — Tue, 15 Nov 2016 13:46:03 +0000

In maths, "trivial" means "easy to reproduce". Doesn't mean it's easy to work out or discover in the first place.

So something like RSA is trivial. BREAKING RSA is "trivial" (in the mathematical sense - it's a well understood problem with a well understood solution). But it's almost impossible to actually do.

Cheers,
Wol

Rant about unprofessional workflow

Wol — Thu, 10 Nov 2016 18:02:05 +0000

The trouble is, (a) managing the kernel is like herding cats, and (b) many of them are blind :-) Without a spec it's too easy to make a mistake, and nobody wants to specify or document what *should* happen, so all too often it doesn't ...

Cheers,
Wol

Rant about unprofessional workflow

bronson — Mon, 07 Nov 2016 18:03:41 +0000

Furthermore, if you're iterating so fast that creating a tracker entry is an unbearable overhead... I'm not sure your work belongs in the stable branch.

Rant about unprofessional workflow

madscientist — Mon, 07 Nov 2016 17:27:01 +0000

Merge errors should be tagged with the tracker entry which introduced the merge error, so that an attempt to cherry-pick the broken commit has a chance of noticing the commit that fixes the error as well.

For whitespace cleanups and typos, you can have a catch-all tracker entries (as mentioned above) for "whitespace cleanup" and "comment typos" that is just used for all those types of issues. It incentivizes people to break these out into separate commits anyway, which is a good thing.

When I introduced a similar requirement locally (using Git hooks on the server to reject pushes where the comment didn't provide the proper information) I originally thought it would be good to have an "out" where you could use a special tag like "trivial" or something and avoid creating a tracker entry. Others in the team were against this and wanted to require a real tracker entry for every commit so that's what we did, and it has been fine.

Rant about unprofessional workflow

NAR — Mon, 07 Nov 2016 16:31:03 +0000

Whitespace, typo fixes, merge errors - they don't warrant issue tracker entries. The malloc/delete mismatches could be real, user facing errors (e.g. destructor not called), these should be tracked.

Rant about unprofessional workflow

excors — Mon, 07 Nov 2016 14:34:54 +0000

If you require every commit to reference an issue, then for trivial commits it might take a couple of minutes to copy a few lines from the commit message into the issue tracker which nobody will ever look at again, so it's wasted time. (For multiple closely-related commits you could combine them into a single issue, like "Fix coding style in xyzzy driver", so it's really an overhead per group of related work, not per commit.)

But if you don't require every commit to reference an issue (and enforce that requirement), someone will occasionally make an important substantive commit without an issue, which will turn out to introduce some bugs or need backporting or need some documentation written about its rationale etc, and someone will have to spend hours searching through mailing lists and LWN articles trying to find all the possibly-years-old discussions about that commit (earlier versions, code reviews, related bug reports, etc). That's also wasted time, which could have been saved if the discussion about the commit had been tracked in a single place (even just as a collection of links to mailing list threads).

If you require only substantive commits to reference an issue, you'll waste time deciding and then arguing with people over whether a commit is really substantive (e.g. a malloc/free bug could be a serious security vulnerability so surely that counts?), and you'll still sometimes fail to track a commit that should have been tracked.

I think it's unclear which approach wastes the most time overall, but the most predictable is to require an issue for every commit - it's a mildly irritating overhead when writing patches, but it's easy to get into the habit of using the issue tracker for everything, and it can save some rare but substantial headaches in the future.

Rant about unprofessional workflow

mathstuf — Mon, 07 Nov 2016 12:19:22 +0000

Every substantive change, sure, maybe a mention in an issue tracker is important, but for issues like malloc/delete mismatches, whitespace fixes, doc typo fixes, and the like, it is just extra work. That's why "every commit requires an issue" is block headed. Maybe every topic/merge request, but certainly not every commit.

A discussion on stable kernel workflow issues

blackwood — Sun, 06 Nov 2016 18:48:54 +0000

Greg apparently already reviews all patches with "Fixes:" tags and cherry-picks for stable what he deems appropriate ...

Rant about unprofessional workflow

NAR — Sun, 06 Nov 2016 14:36:08 +0000

It should not be much work to submit a bug report. Less than 5 minutes. And it usually helps to see the bug description in text, because the developer might realize what else is affected (or it's not a bug at all or some logs are missing, etc.). And it really helps a year later when somebody tries to find out why that particular change went into the codebase.

I don't really understand the comment about feature branch: if it's a bug in the new (not yet released) feature, then there should be already an entry in the issue tracker about that feature. If it's a bug in an old (already released) feature, then it should be present on the "main" branch too, so it warrants a new bug entry.

Rant about unprofessional workflow

imMute — Fri, 04 Nov 2016 16:22:42 +0000

>Granted, this needs discipline. No cowboy-style commit-and-forget programming. For example if a developer finds a bug during code review or implementing some other feature, he had to create the bug report and go through the process - but in the long run it was really useful.

And here's where people would just sit on uncommitted code because its too much work having to fill out a bug report on code that is still on a feature branch. If it's a bug found by the second set of eyes to see the code, then I don't think a bug report is worth it - it's just needless paperwork.

A discussion on stable kernel workflow issues

jani — Thu, 03 Nov 2016 13:24:51 +0000

Indeed. I think I'm starting to lean towards preferring Fixes: tags over Cc: stable at the time of applying patches, and doing separate stable backport requests later, when we've had more exposure to the candidate fixes. The stable backport requests could be scripted using the Fixes: tags, checking that the later commits don't contain reverts of the "fix" or commits fixing the "fix".

A discussion on stable kernel workflow issues

spender — Thu, 03 Nov 2016 12:08:49 +0000

Since the above comment is now being linked to from oss-sec regarding a CVE request for the vuln (though oddly, for the infoleak itself and not the bad fix that results in privilege escalation) I want to add that I've verified that the KERNEXEC plugin also prevents exploitation.

I do like though that the infoleak will receive a CVE, since simply not fixing the infoleak (which could have been done very easily in combination with reverting the "problematic" commits) is unacceptable. We have a very elegant fix for it in grsecurity that doesn't rely on either commit.

Also, since this covering up of security vulnerabilities has really gotten out of hand, we have finally started listing on our website an added benefit for customers that was previously unpublicized: we routinely send out notices for silently-fixed vulnerabilities with detailed analysis and how grsecurity affects exploitation. Mails were sent out to customers for this vulnerability as well as DirtyCOW for instance, the day the commits were public. I will note the information we provided answered at least one of solar's "challenges" and stood in constrast to much of the optimistic misinformation that was being spread about the vulnerability.

Greg KH has publicly alluded that he wants to damage our ability to make a living from our work. Greg, I would be happy for you to make this item of our services irrelevant by committing in the future to stop lying to your users.

-Brad

A discussion on stable kernel workflow issues

syrjala — Thu, 03 Nov 2016 00:42:28 +0000

One thing I'd like to see is some kind of central database of commits that should *not* be backported to stable, even if they have the cc:stable tag. We've had a few cases in i915 land where the cc:stable was possibly applied a bit too optimistically, or perhaps the patch really seemed like stable material at the time but was later found to cause a regression. It's quite tedious having to NAK the same patch over and over again when various people proceed to backport it to whatever stable branch they maintain. One NAK should be enough to block it from all stable trees IMO.

A discussion on stable kernel workflow issues

rich0 — Wed, 02 Nov 2016 13:24:10 +0000

I agree that there needs to be a better way to communicate things like this.

I was working on updating a system to 4.4.29, and then I heard from somebody on IRC I think that somebody had mentioned at the summit to not bother as it had some issue and 4.4.30 was going to be released shortly.

When the -announce list says "upgrade your systems" and the "wait, don't upgrade your systems" is part of the Q&A at some conference, there is a problem somewhere.

I fully get that sometimes there is going to be a vulnerability and you can't always wait to do updates. However, it seems like the last few weeks have had far more updates than can be explained by this. Preventing bugs from ever making their way into mainline is one challenge, but avoiding regressions in stable seems like another.

A discussion on stable kernel workflow issues

daenzer — Wed, 02 Nov 2016 11:16:47 +0000

My experience with the stable kernel workflow hasn't been bad overall, but there have been exceptions.

My worst experience so far was when I received "stable review patch" e-mails for backporting e64c952efb8e0c15ae82cec8e455ab4910690ef1 ("drm/radeon: disable runtime pm on PX laptops without dGPU power control") to 4.4 and 4.5. I followed up as soon as I could, reporting that a regression had been bisected to that commit, so it would be better to hold off on backporting it. Greg KH's response to that was "Why not just provide stable with the bugfix as well?", but there was no such fix yet at that time. The change was backported anyway, which resulted in at least one additional bug report (and probably more people unnecessarily running into the regression).

It left me wondering what the purpose of those "stable review patch" e-mails was.

Rant about unprofessional workflow

NAR — Wed, 02 Nov 2016 10:58:43 +0000

I worked on projects where the rule was that no commit should go into the main repository without a reference to the bug or task tracker. All commits should be either fixes for bugs (in this case the reference should point to the bug in question) or implementation of new feature (in this case the reference should point to the task tracker describing the new feature). Code cleanups, refactorings, etc. were considered "new feature" for this purpose. There was a field on the bug tracker ("introduced at") where the developer who fixed the bug had to state which version introduced the bug. This helped greatly the decision when we needed to backport a fix or not. Also the bug description obviously described if this is a severe bug (e.g. crash, security problem, etc.) or not.

Granted, this needs discipline. No cowboy-style commit-and-forget programming. For example if a developer finds a bug during code review or implementing some other feature, he had to create the bug report and go through the process - but in the long run it was really useful. I guess the kernel developers don't think that there's a bug or issue tracker that can cope with their workloads (hint: write one).

A discussion on stable kernel workflow issues

citypw — Wed, 02 Nov 2016 10:18:23 +0000

oops, guess I misunderstand it...s0rry guys...

A discussion on stable kernel workflow issues

intgr — Wed, 02 Nov 2016 08:25:09 +0000

> Trivial root exploit? I wouldn't say any privilege escalation exploit is trivial;-)

I assumed "trivial" means trivially easy to exploit... I doubt anyone meant to say that it's unimportant.

A discussion on stable kernel workflow issues

citypw — Wed, 02 Nov 2016 08:00:23 +0000

The truth of another "silent fix" ( again-_-)! Awesome, it seems vanilla kernel doesn't care if GNU/Linux users( individual/enterprise/etc) have important digital asset on those 0ld good machines without PaX/Grsecurity's KERNEXEC/UDEREF or SMEP/SMAP. Lacking mitigation to prevent 0ld shitty ret2usr exploit is one thing, while "a trivial root exploit" is another. Trivial root exploit? I wouldn't say any privilege escalation exploit is trivial;-)

A discussion on stable kernel workflow issues

spender — Wed, 02 Nov 2016 00:15:49 +0000

Unless the answer is dogmatic adherence to a new policy of reverting things that are fixed perfectly well, it's not.

-Brad

A discussion on stable kernel workflow issues

jake — Wed, 02 Nov 2016 00:05:18 +0000

> For whatever reason (I guess we would have to be present at the kernel summit to know)
> they decided to revert both patches and leave the infoleak there for 4.4.30 instead.

I think the answer you seek is in the article you are commenting on, Brad.

jake

A discussion on stable kernel workflow issues

spender — Tue, 01 Nov 2016 23:19:34 +0000

To be specific, 4.4.29 is not vulnerable. It includes the exception table handling patch. For whatever reason (I guess we would have to be present at the kernel summit to know) they decided to revert both patches and leave the infoleak there for 4.4.30 instead.

-Brad

A discussion on stable kernel workflow issues

spender — Tue, 01 Nov 2016 22:22:46 +0000

And to spell it all out:

For the 4.4.x -stable series, the vulnerability affected 4.4.22 to 4.4.29 -- all 4.4 stable kernels from September 24th onward, including the ones everyone updated to to fix DirtyCOW.

Presumably the "older CPUs" refers to pre-Ivy Bridge which lacked SMEP, as the broken fixup results in a jump to a userland address. This post: http://www.mail-archive.com/linux-kernel@vger.kernel.org/... shows a reproducer and the OOPS illustrating the userland instruction pointer address. Grsecurity's i386 KERNEXEC and x64 UDEREF would both prevent this from being exploitable, mmap_min_addr would do nothing.

-Brad

A discussion on stable kernel workflow issues

bronson — Tue, 01 Nov 2016 22:22:38 +0000

And it's ridiculous that this is how the word gets out. I'm really struggling to understand the muddled thinking and communication that have gone into the last few days of stable kernel development.

A discussion on stable kernel workflow issues

alavaliant — Tue, 01 Nov 2016 22:14:02 +0000

I wonder what the 'older CPUs' types are? The number of machines that can be exploited would differ greatly depending on if the effected cpu classes are ones older than 2 years or ones older than 10 years.

A discussion on stable kernel workflow issues

spender — Tue, 01 Nov 2016 21:42:04 +0000

So now we have the answer to the "a bug" fixed in 4.4.30:

"The stable trees recently shipped a three-line patch that made a trivial root exploit possible on older CPUs."

This is ridiculous.

-Brad