6 ms·

Here is the commit that introduces the bug: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=74d46992e0d9 https://git.kernel.org/pu

Here is the commit that introduces the bug: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=74d46992e0d9 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/lin... Can you spot the error? Here is the commit that fixes the bug: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=62530ed8b1d https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/lin...

lomnakkus 9y ago
Why is this a single commit? I would expect renaming like that to be done in completely separate commits and then subsequent commits to do any fixups. Is it just a merge commit? I imagine it's probably unfair to blame Linus for this (assuming it's a huge merge), but it would also weird for you to point to a merge commit as evidence of the opposite. Color me confused.
- vog 9y ago
  Not sure why you are downvoted. I thought exactly the same thing after looking at the commit. As a software developer, the habit committing smaller changes has saved me a lot of time. Even more so, a clean distinction between refactorings (e.g. moving or renaming things) and functional changes (e.g. adding or fixing things) is vital to any large code base. As the saying goes: "for each desired change, make the change easy (warning: this may be hard), then make the easy change" (Kent Beck)
  - blueflow 9y ago
    I wonder what the creators of git think about your opinion on how to use it properly... Jokes aside, what you see it usually just the squashed commit submitted for merge. With 6 million git objects, you reconsider twice having each change in a single commit.
    - bonzini 9y ago
      That's absolutely wrong. Linux never squashes merge commits. With my Linux maintainer hat on, I would have never accepted that commit.
      - deleted 9y ago
        [deleted]
    - lomnakkus 9y ago
      > I wonder what the creators of git think about your opinion on how to use it properly... Are you the creator of git? If not, how exactly are you qualified to judge "proper" usage -- by YOUR standard you're not qualified yourself, so...? (I realized it was a joke, but I offer the above as a counterpoint to the satirical/ironic element of the joke. My opinion is that the originator doesn't get special treatment. But then the point wasn't git, it was what/how things get merged into the Linux kernel. The mechanism is entirely immaterial.) Squashing commits for merge into Linux is not standard practice AFAIK... and happily I see an Linux committer has async'ly backed me up on this. Generally the preference I see, where git is concerned (not sure about the kernel), is that everything is rebased right into the master branch to avoid merge graphs getting absurd.
      - blueflow 9y ago
        I meant squashing before sending upstream, not all kernel development happens in public. Still, i think squashing still happens in the public trees: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8c609698569578913ad40bb160b97c3f6cfa15ec https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/lin... EDIT: Actually, thats indeed a merge, but the diff is still showed with it, which confused me (because conflict-less merges are usually +0/-0).
        lomnakkus 9y ago
        OK, that's fair enough. Let's not be enemies :)
    - vog 9y ago
      > what you see it usually just the squashed commit submitted for merge Then __david__'s argument "could you spot the error?" is moot, because the squashed commit (hopefully!) was not the subject of review, but the smaller commits.
- kelnos 9y ago
  The "GitHub model" is very different from the original kernel-dev model of using git. GitHub encourages you to make a bunch of smaller commits and then roll them all up into one pull request -- so the unit of change is the PR, and not so much the single commit. In kernel-dev land, the unit of change is the commit. Sometimes larger changes will be broken up into a series of smaller commits, but in general you tend to see single-commit changes (which often come from a series of commits someone has done and then squashed down to one).
  - bonzini 9y ago
    No, the kernel definitely encourages submitting patch series instead of large changes. That's why git has the "git am" (apply mbox) command. Patches that are submitted by external contributors are never squashed by maintainers (except to occasionally fix a bug that was discovered after the submission).
  - lomnakkus 9y ago
    You're wrong. Sorry. Read up on it -- or, just, look at the kernel mailing list.
- nickodell 9y ago
  This isn't just a rename. The structure dm_bio_details was changed to remove one field that points to a block_device structure, and add a field that points to a gendisk structure, and an integer called bi_partno. This change copies the gendisk structure, but not bi_partno. That's the bug.
  - lomnakkus 9y ago
    AFAIU the commits there was a rename and then a remove. Am I wrong in that assessment?
    - mlyle 9y ago
      The important distinction is where the critical information on where the device offset is encoded from. Before it was specified by whether a partition or parent disk device was used; now it is always the actual disk and a partition number. It's not a bad change, but IMO it was made recklessly. From this patchset, there's now at least four different fixes: tags referring to it in Linus's tree. The patchset itself was broken and intermediate states didn't compile, making bisection annoying. And there's many other callers of this function that are potentially getting bad results, along with other cousins like bio_clone_bioset that are still unfixed. This was not a fun experience for a new maintainer. It turns out if you had created a fresh bcache on 4.14, like most of my tests do-- you're probably fine. And not all environments will experience problems. It went in a couple of weeks before I became maintainer, and isn't in the bcache code itself (and wasn't reviewed by anyone who worked on bcache, and was never sent to the linux-bcache mailing list.) I've learned some new things to add to my test suite, and I guess i need to read all the block diff to make sure people don't break things out from under me.
      - vog 9y ago
        > The patchset itself was broken and intermediate states didn't compile As a maintainer, isn't it then your right to refuse the patchset and demanding a clean patchset instead?
        9y ago
- ambrop7 9y ago
  Possibly so that the code corresponding to each commit builds/works, which makes bisect much easier.
StreamBright 9y ago
You are just proving the point that opensource wont protect against security bugs because most of the software engineers cannot spot a security bug in code?
- mort96 9y ago
  I'm not sure if anyone has ever claimed that open source is a guaranteed protection against every security bug. Besides, this wasn't a security bug, was it? It corrupted the filesystem, it didn't give anyone else any undesired access did it?
- PopsiclePete 9y ago
  And you are "proving the point" that humans are fallible and make mistakes and "open source" never meant, and can't mean, that the code is magically of better quality?
- yjftsjthsd-h 9y ago
  Given the response time, I would say that this is an amazing example of the protection that open source provides.
- chris_wot 9y ago
  What security bug? It was a corruption bug.
StillBored 9y ago
A part of this is the "Blessed maintainer" issue, where large patchsets like this that a mere moral submits would end up going through dozens of review/testing cycles before even being considered by the maintainers of most subsystems. OTHO, the maintainer of said subsystem can write these huge patches, get one of his buddies to ack it, and its in the tree without much comment/testing. As this bug points out, being a maintainer doesn't suddenly make all your code bug free.. EDIT: OTOH, most of the Linus released kernels go through months of bug fixing before they show up in any of the stable distributions. Its the "rolling release" distro's which get hit by these bugs because they have this strange idea that once Linus (or whatever package) releases something its done cooking.

lomnakkus 9y ago

Why is this a single commit? I would expect renaming like that to be done in completely separate commits and then subsequent commits to do any fixups. Is it just a merge commit? I imagine it's probably unfair to blame Linus for this (assuming it's a huge merge), but it would also weird for you to point to a merge commit as evidence of the opposite. Color me confused.

vog 9y ago
Not sure why you are downvoted. I thought exactly the same thing after looking at the commit. As a software developer, the habit committing smaller changes has saved me a lot of time. Even more so, a clean distinction between refactorings (e.g. moving or renaming things) and functional changes (e.g. adding or fixing things) is vital to any large code base. As the saying goes: "for each desired change, make the change easy (warning: this may be hard), then make the easy change" (Kent Beck)
- blueflow 9y ago
  I wonder what the creators of git think about your opinion on how to use it properly... Jokes aside, what you see it usually just the squashed commit submitted for merge. With 6 million git objects, you reconsider twice having each change in a single commit.
  - bonzini 9y ago
    That's absolutely wrong. Linux never squashes merge commits. With my Linux maintainer hat on, I would have never accepted that commit.
    - deleted 9y ago
      [deleted]
  - lomnakkus 9y ago
    > I wonder what the creators of git think about your opinion on how to use it properly... Are you the creator of git? If not, how exactly are you qualified to judge "proper" usage -- by YOUR standard you're not qualified yourself, so...? (I realized it was a joke, but I offer the above as a counterpoint to the satirical/ironic element of the joke. My opinion is that the originator doesn't get special treatment. But then the point wasn't git, it was what/how things get merged into the Linux kernel. The mechanism is entirely immaterial.) Squashing commits for merge into Linux is not standard practice AFAIK... and happily I see an Linux committer has async'ly backed me up on this. Generally the preference I see, where git is concerned (not sure about the kernel), is that everything is rebased right into the master branch to avoid merge graphs getting absurd.
    - blueflow 9y ago
      I meant squashing before sending upstream, not all kernel development happens in public. Still, i think squashing still happens in the public trees: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8c609698569578913ad40bb160b97c3f6cfa15ec https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/lin... EDIT: Actually, thats indeed a merge, but the diff is still showed with it, which confused me (because conflict-less merges are usually +0/-0).
      - lomnakkus 9y ago
        OK, that's fair enough. Let's not be enemies :)
  - vog 9y ago
    > what you see it usually just the squashed commit submitted for merge Then __david__'s argument "could you spot the error?" is moot, because the squashed commit (hopefully!) was not the subject of review, but the smaller commits.
kelnos 9y ago
The "GitHub model" is very different from the original kernel-dev model of using git. GitHub encourages you to make a bunch of smaller commits and then roll them all up into one pull request -- so the unit of change is the PR, and not so much the single commit. In kernel-dev land, the unit of change is the commit. Sometimes larger changes will be broken up into a series of smaller commits, but in general you tend to see single-commit changes (which often come from a series of commits someone has done and then squashed down to one).
- bonzini 9y ago
  No, the kernel definitely encourages submitting patch series instead of large changes. That's why git has the "git am" (apply mbox) command. Patches that are submitted by external contributors are never squashed by maintainers (except to occasionally fix a bug that was discovered after the submission).
- lomnakkus 9y ago
  You're wrong. Sorry. Read up on it -- or, just, look at the kernel mailing list.
nickodell 9y ago
This isn't just a rename. The structure dm_bio_details was changed to remove one field that points to a block_device structure, and add a field that points to a gendisk structure, and an integer called bi_partno. This change copies the gendisk structure, but not bi_partno. That's the bug.
- lomnakkus 9y ago
  AFAIU the commits there was a rename and then a remove. Am I wrong in that assessment?
  - mlyle 9y ago
    The important distinction is where the critical information on where the device offset is encoded from. Before it was specified by whether a partition or parent disk device was used; now it is always the actual disk and a partition number. It's not a bad change, but IMO it was made recklessly. From this patchset, there's now at least four different fixes: tags referring to it in Linus's tree. The patchset itself was broken and intermediate states didn't compile, making bisection annoying. And there's many other callers of this function that are potentially getting bad results, along with other cousins like bio_clone_bioset that are still unfixed. This was not a fun experience for a new maintainer. It turns out if you had created a fresh bcache on 4.14, like most of my tests do-- you're probably fine. And not all environments will experience problems. It went in a couple of weeks before I became maintainer, and isn't in the bcache code itself (and wasn't reviewed by anyone who worked on bcache, and was never sent to the linux-bcache mailing list.) I've learned some new things to add to my test suite, and I guess i need to read all the block diff to make sure people don't break things out from under me.
    - vog 9y ago
      > The patchset itself was broken and intermediate states didn't compile As a maintainer, isn't it then your right to refuse the patchset and demanding a clean patchset instead?
      - 9y ago
ambrop7 9y ago
Possibly so that the code corresponding to each commit builds/works, which makes bisect much easier.

StreamBright 9y ago

You are just proving the point that opensource wont protect against security bugs because most of the software engineers cannot spot a security bug in code?

mort96 9y ago
I'm not sure if anyone has ever claimed that open source is a guaranteed protection against every security bug. Besides, this wasn't a security bug, was it? It corrupted the filesystem, it didn't give anyone else any undesired access did it?
PopsiclePete 9y ago
And you are "proving the point" that humans are fallible and make mistakes and "open source" never meant, and can't mean, that the code is magically of better quality?
yjftsjthsd-h 9y ago
Given the response time, I would say that this is an amazing example of the protection that open source provides.
chris_wot 9y ago
What security bug? It was a corruption bug.

StillBored 9y ago

A part of this is the "Blessed maintainer" issue, where large patchsets like this that a mere moral submits would end up going through dozens of review/testing cycles before even being considered by the maintainers of most subsystems. OTHO, the maintainer of said subsystem can write these huge patches, get one of his buddies to ack it, and its in the tree without much comment/testing. As this bug points out, being a maintainer doesn't suddenly make all your code bug free.. EDIT: OTOH, most of the Linus released kernels go through months of bug fixing before they show up in any of the stable distributions. Its the "rolling release" distro's which get hit by these bugs because they have this strange idea that once Linus (or whatever package) releases something its done cooking.