Hacker News Reader

Comment by __david__ | Hacker News Reader

Here is the commit that introduces the bug: 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/lin...

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.

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)

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.

That's absolutely wrong. Linux never squashes merge commits.

With my Linux maintainer hat on, I would have never accepted that commit.

> 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.

> 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.

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/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).

OK, that's fair enough. Let's not be enemies :)

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.

AFAIU the commits there was a rename and then a remove. Am I wrong in that assessment?

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.

> 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?

He's a maintainer of bcache, not of block layer, where the problematic patchset was introduced.

As pointed out elsewhere, I never accepted this patchset. But:

For stuff I accept, I try and make sure that intermediate states compile, but I can't promise I run a compile on every intermediate step or that my review eye is good enough to spot every wrongly-ordered-dependent-change. It would be nice if someone would throw hardware at building intermediate states of linux-next and sent nastygrams to maintainers/committers about it: might make people more careful.

Can't you use GitHub's integrated TravisCI for that? At least that's what I know other Free Software projects are doing.

TravisCI generally tests the end result of a PR, not all intermediate states.

I have a continuous integration tool for bcache in place, but it's more geared on end states-- compiling/checkpatch/test sequence.

i ran coccinelle on the original code before the patch with this rule to see if it would spot any other missing changes.

      linux git:(c2ee070fb003)  cat patch.coci
    @@
    expression lhs;
    expression rhs;
    @@

    - lhs->bi_bdev = rhs;
    + bio_set_dev(bio, rhs);

so it spotted the original missing bi_partno = in the same file.

        diff -u -p a/block/bio.c b/block/bio.c
    --- a/block/bio.c
    +++ b/block/bio.c
    @@ -596,7 +596,7 @@ void __bio_clone_fast(struct bio *bio, s
       * most users will be overriding ->bi_bdev with a new target,
       * so we don't set nor calculate new physical/hw segment counts here
       */
    - bio->bi_bdev = bio_src->bi_bdev;
    + bio_set_dev(bio, bio_src->bi_bdev);
      bio_set_flag(bio, BIO_CLONED);
      bio->bi_opf = bio_src->bi_opf;
      bio->bi_write_hint = bio_src->bi_write_hint;
    @@ -681,7 +681,7 @@ struct bio *bio_clone_bioset(struct bio
      bio = bio_alloc_bioset(gfp_mask, bio_segments(bio_src), bs);
      if (!bio)
        return NULL;
    - bio->bi_bdev    = bio_src->bi_bdev;
    + bio_set_dev(bio, bio_src->bi_bdev);
      bio->bi_opf   = bio_src->bi_opf;
      bio->bi_write_hint  = bio_src->bi_write_hint;
      bio->bi_iter.bi_sector  = bio_src->bi_iter.bi_sector;

but it also spotted a missing assignment in the same file in the bio_clone_bioset function. i don't know enough about this functionality to know if the assignment is meant to be here as well. also, considering it is in the same file i would have thought if it was necessary it would have been noticed in the last patch. just thought i would let someone else who would have a better idea about how this works know.

i also haven't checked the rest of the coccinelle output. i know it has generated a lot of _wrong_ code. like the actual patch looks like it has done something more intelligent than what coccinelle blindly did.

Read. :P

To quote what you replied to: "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."

bcache doesn't call bio_clone_bioset. I put in the minimum change to fix the bcache regression, for quick review and acceptance. You're free to submit this to linux-block if you want.

thanks :) i actually didn't read the post at all. heh

Possibly so that the code corresponding to each commit builds/works, which makes bisect much easier.

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).

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).

You're wrong. Sorry. Read up on it -- or, just, look at the kernel mailing list.

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.

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?

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?

Given the response time, I would say that this is an amazing example of the protection that open source provides.

What security bug? It was a corruption bug.

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?

Reply on news.ycombinator.com