Precedence bug in patch

Bug #86778 reported by Blaisorblade
8
Affects Status Importance Assigned to Milestone
linux (Ubuntu)
Fix Released
Undecided
Unassigned
Declined for Edgy by Brian Murray
Dapper
Invalid
Undecided
Unassigned
linux-source-2.6.15 (Ubuntu)
Invalid
Undecided
Unassigned
Declined for Edgy by Brian Murray
Dapper
Fix Released
Medium
Colin Ian King
linux-source-2.6.17 (Ubuntu)
Invalid
Undecided
Unassigned
Declined for Edgy by Brian Murray
Dapper
Invalid
Undecided
Unassigned

Bug Description

Binary package hint: linux-source-2.6.15

When compiling linux-source, I got an unexpected warning, which turned out to be caused by these two commits (links are per Dapper and Edgy):

http://www2.kernel.org/git/?p=linux/kernel/git/bcollins/ubuntu-dapper-updates.git;a=commitdiff;h=622f7b4c920a9362d506400c0da02e3e06442b27
http://www2.kernel.org/git/?p=linux/kernel/git/bcollins/ubuntu-edgy.git;a=commitdiff;h=622f7b4c920a9362d506400c0da02e3e06442b27

In particular, since '>>' has lower precedence than +, the below hunk doesn't do what it wants to do. I've not tested which bad things can happen, but it warrants a fix. In fact, in mainline the missing parentheses have been added - see commit ac924c6034d9095f95ee889f7e31bbb9145da0c2, merged in 2.6.17.

@@ -2462,8 +2464,8 @@ void setup_per_zone_pages_min(void)
                        zone->pages_min = tmp;
                }

- zone->pages_low = zone->pages_min + tmp / 4;
- zone->pages_high = zone->pages_min + tmp / 2;
+ zone->pages_low = zone->pages_min + tmp >> 2;
+ zone->pages_high = zone->pages_min + tmp >> 1;
                spin_unlock_irqrestore(&zone->lru_lock, flags);
        }
 }

Revision history for this message
Blaisorblade (p-giarrusso) wrote :

This is the needed patch. Btw, to trigger this problem it seems you need to fiddle with /proc.

Revision history for this message
Gareth Fitzworthington (mapping-gp-deactivatedaccount) wrote :

This report has had no activity for a considerable period.
Is this still of importance?
It appears to be a compile issue (perhaps unique) associated with a patch extrinsic to standard delivery channels.
Possibly now superseded??

Changed in linux-source-2.6.17:
status: New → Incomplete
Changed in linux-source-2.6.15:
status: New → Incomplete
Revision history for this message
Blaisorblade (p-giarrusso) wrote :

It applies to 6.06 LTS, so it is still relevant. And it's trivial to solve (for a kernel developer it should take two minutes to agree with me, IMHO; I'm a kernel developer too).

Back on the issue: it is trivial to solve. Look at the patch you merged (updated URL):
http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-dapper-updates.git;a=commitdiff;h=622f7b4c920a9362d506400c0da02e3e06442b27

and at the patch merged in mainline:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=ac924c6034d9095f95ee889f7e31bbb9145da0c2

The bug I mention was corrected before merging in mainline, so you can fix it with confidence, too. And by reading the hunk I mentioned it's clear that the new code, without parentheses, is wrong, because it should do the same thing as the old code.

A note on what you say: the patch is not "extrinsic to standard delivery channels", it is part of the kernel you package, and is not a "compile issue". It is a real bug (and the warning from the compiler points this out). That patch has been shipped in Dapper & Edgy kernel packages for a long time. The GIT tree I linked is where all development on Ubuntu kernels is tracked (GIT is a Version Control System, similar to CVS, Subversion etc...). I must suppose you are not a kernel developer. Well, then I'll mention that I've been a kernel developer for quite a bit of time so, at the very least, I know what I am talking about.

Changed in linux-source-2.6.17:
assignee: nobody → kernel-team
Changed in linux-source-2.6.15:
assignee: nobody → kernel-team
Changed in linux-source-2.6.15:
assignee: kernel-team → ubuntu-kernel-team
Changed in linux-source-2.6.17:
assignee: kernel-team → ubuntu-kernel-team
Revision history for this message
Sergio Zanchetta (primes2h) wrote :

The 18 month support period for Edgy Eft 6.10 has reached it's end of life. As a result, we are closing the linux-source-2.6.17 Edgy Eft kernel task. However, please note that this report will remain open against the actively developed kernel. Thank you for your continued support and help as we debug this issue.

Changed in linux-source-2.6.17:
status: Incomplete → Invalid
Revision history for this message
Sergio Zanchetta (primes2h) wrote :

Hardy Heron 8.04 was recently released. It would be helpful if you could test the new release and verify if this is still an issue - http://www.ubuntu.com/getubuntu/download . You should be able to test your bug using the LiveCD. Please let us know your results. Thanks.

Changed in linux:
status: New → Incomplete
Revision history for this message
Blaisorblade (p-giarrusso) wrote :

I marked the issue as "confirmed" myself because I am confident of what I said, and that the trivial change I suggested fixes the problem. And any competent C programmer can agree on that, after reading my report.

I avoided whining for long time, but the only conclusion I can give to this is that you have no competent C programmer, in your bug tracking team. Actually, not enough to even read bug reports.

Given that I have sent two kernel bug reports containing the solution, that I'm a kernel hacker myself, and that after more than one year the bug is still open, how will you be able to handle _real_ bug reports, where finding the problem is damn difficult?

Changed in linux:
status: Incomplete → Confirmed
Changed in linux-source-2.6.15:
status: Incomplete → Confirmed
Revision history for this message
Leann Ogasawara (leannogasawara) wrote :

The Ubuntu Kernel Team is planning to move to the 2.6.27 kernel for the upcoming Intrepid Ibex 8.10 release. As a result, the kernel team would appreciate it if you could please test this newer 2.6.27 Ubuntu kernel. There are one of two ways you should be able to test:

1) If you are comfortable installing packages on your own, the linux-image-2.6.27-* package is currently available for you to install and test.

--or--

2) The upcoming Alpha5 for Intrepid Ibex 8.10 will contain this newer 2.6.27 Ubuntu kernel. Alpha5 is set to be released Thursday Sept 4. Please watch http://www.ubuntu.com/testing for Alpha5 to be announced. You should then be able to test via a LiveCD.

Please let us know immediately if this newer 2.6.27 kernel resolves the bug reported here or if the issue remains. More importantly, please open a new bug report for each new bug/regression introduced by the 2.6.27 kernel and tag the bug report with 'linux-2.6.27'. Also, please specifically note if the issue does or does not appear in the 2.6.26 kernel. Thanks again, we really appreicate your help and feedback.

Changed in linux:
status: New → Invalid
Changed in linux-source-2.6.15:
assignee: nobody → ubuntu-kernel-team
importance: Undecided → Medium
status: New → Triaged
Changed in linux-source-2.6.17:
status: New → Invalid
Changed in linux:
status: Confirmed → Fix Released
Revision history for this message
Leann Ogasawara (leannogasawara) wrote :

I've approved the Dapper nomination and will bring to the attention of the kernel team. Thanks.

Changed in linux-source-2.6.15:
assignee: ubuntu-kernel-team → colin-king
milestone: none → dapper-updates
status: Triaged → In Progress
Revision history for this message
Colin Ian King (colin-king) wrote :

SRU Justification:

Impact: The mm zone page calculations are being miscalculated because of
a previous commit.

Fix: Add parentheses around >> operators in setup_per_zone_pages_min()
to correct the calculation. Elaboration here:

originally the calculation was:

zone->pages_low = zone->pages_min + tmp / 4;
zone->pages_high = zone->pages_min + tmp / 2;

commit 622f7b4c920a9362d506400c0da02e3e06442b27 changed it to:

zone->pages_low = zone->pages_min + tmp >> 2;
zone->pages_high = zone->pages_min + tmp >> 1;

unfortunately the + operator has higher precedence than >> hence the
bug.

it should be:

zone->pages_low = zone->pages_min + (tmp >> 2);
zone->pages_high = zone->pages_min + (tmp >> 1);

Testcase: One can see the zonelist from /proc/zoneinfo: The low and high
water marks should be different between the buggy and fixed versions.

Testing:

1. Install 6.0.6 LTS i386 Server in VirtualBox and
recording /proc/zoneinfo from /etc/rc.local to capture the zoneline low
and high settings at boot time.
2. Booted up 3 times to see if the figures differ per boot (they
don't).
3. Installed the fixed kernel and repeated the 3 boot test
4. Compare the buggy and fixed /proc/zoneinfo logs, e.g:

        Buggy Fixed
low 15 38
high 31 46

Attached: zone info logs for the two cases

Revision history for this message
Colin Ian King (colin-king) wrote :
Changed in linux-source-2.6.15:
status: In Progress → Fix Committed
Revision history for this message
Blaisorblade (p-giarrusso) wrote :

Thank you for fixing this bug. This improves Ubuntu's reputation in my view.
===
Btw, what was the key to get an answer from you? As you see, the previous lack of answers (for 18 months) had led me to angry and questionable comments, which I can now retract, such as:

"I avoided whining for long time, but the only conclusion I can give to this is that you have no competent C programmer, in your bug tracking team. Actually, not enough to even read bug reports."

I guess that something has to improve to avoid such things. Should I try subscribing to some -dev ML and discussing the issue there (if I find the needed time)?

Thanks for the attention.
Regards

Revision history for this message
Martin Pitt (pitti) wrote :

Accepted into -proposed, please test and give feedback here. Please see https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Thank you in advance!

Martin Pitt (pitti)
Changed in linux-source-2.6.15:
status: Confirmed → Invalid
Revision history for this message
Blaisorblade (p-giarrusso) wrote :

I'm sorry, but I can't test it, nor I feel like I should. I hope that's not a problem for including the fix in the kernel.

I have no 6.06 system at hand right now, plus I reported the bug just because I saw a compiler warning and investigated it, simply by source code review (compare source code with mainline, search history to explain differences). The testing done by Colin King is already more than enough, even because this is not a driver and static analysis here is enough (and I say that also because of my past experience as kernel developer).

Thank you

Revision history for this message
Steve Beattie (sbeattie) wrote :

I have reproduced the issue with the linux kernel package 2.6.15-52.71 from dapper-security and can confirm that the version in dapper-proposed, 2.6.15-52.72, does indeed address the issue.

On my test system, the low settings ended up being smaller than the min setting, while the high settings ended up being equal to the min. In the fixed version, both the high and low settings were larger than min, as it should be.

Revision history for this message
Martin Pitt (pitti) wrote :

Copied to dapper-updates.

Changed in linux-source-2.6.15:
status: Fix Committed → Fix Released
Revision history for this message
Launchpad Janitor (janitor) wrote : Kernel team bugs

Per a decision made by the Ubuntu Kernel Team, bugs will longer be assigned to the ubuntu-kernel-team in Launchpad as part of the bug triage process. The ubuntu-kernel-team is being unassigned from this bug report. Refer to https://wiki.ubuntu.com/KernelTeamBugPolicies for more information. Thanks.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.