Hacker News Reader

Comment by kazinator | Hacker News Reader

The maintainer's patch is better.

1. It doesn't lose the fpidx < (PT_FPSCR - PT_FPR0) test from the 32 bit case.

2. In the put function, it doesn't lose the *data = child->thread.fp_state.fpscr; fallback assignment, which is taken when fpidx is out of bounds.

3. It avoids the pointless FPRINDEX macro which reduces to identity if TS_FPRWIDTH is 1. The original patch's commit message says "On PPC32 it's ok to assume that TS_FPRWIDTH is 1", but then includes this anyway:

  #define FPRNUMBER(i) (((i) - PT_FPR0) >> 1)
  #define FPRHALF(i) (((i) - PT_FPR0) & 1)
  #define FPRINDEX(i) TS_FPRWIDTH * FPRNUMBER(i) * 2 + FPRHALF(i)

Sorry, just because you found this trivially fixable array mismatch doesn't mean your buggy patch has to be accepted the way it is.

The maintainer should, however, acknowledge that Miculas didn't simply report the problem, but that Miculas investigated identified the root cause, and had his own version of almost the same simple fix.

"Reported-By" lacks the precision to express that it was thoroughly investigated by that party, and root-caused.

View on HN

I agree that the maintainers work is better. But he could have just done this as a code review on the patch and ask for an update.

That happens constantly for new contributors to a project and it is the proper way to do things - it teaches people and brings them up to speed. If you just redo their work, you are discouraging them from contributing while also not allowing for a teachable moment.

Not on a security related list though. That's entirely the wrong expectation.

Security through obscurity? Oh...

linuxppc-dev is not a security list.

It was cc-'d to the security list.

On the first patch that Miculas submitted with their gmail address that was not looked at by any of the parties involved in the rest of the story. The subsequent mails to linuxppc-dev where Leroy and Ellerman were involved were not cross-posted AFAICT.

Yes, that's correct. But the OP didn't have to CC the security mailing list in the first place and that changed the dynamic considerably.

Note that the degree to which this has blown up is disproportional and that besides the fact that Michael Ellerman could have handled this more gracefully the OP did not seek to resolve this out of band, misrepresented the interaction and ignored an olive branch that would have given him exactly what he wanted.

The bit where I am in strict disagreement with how Michael Ellerman handled this is actually the reverse of what happened: if the original patch wasn't properly signed off on and the OP was responsive they actually put themselves at risk by re-using the code without proper attribution. This - and that's speculative - probably was because the code identified the essence of the fix but introduced a couple of new problems and because it was so small he may have felt that fixing it and adding a 'Reported-by' tag plus the LKML archive would be sufficient cover for that. But from a strict reading of the LKML guidelines that was the wrong thing to do.

And the LKML guidelines themselves should be made clearer with respect to how the various levels of contribution are dealt with because that seems to matter a lot to some people and they need to spell out more clearly that submitting a patch may result in your code ending up in the kernel with LKML as the only visible proof that this is the case. Because the OP may not realize it but that is the essential part: that their submission to LKML ended with a bug in the Linux kernel fixed and there is ample proof to document that and I'm pretty sure that nobody would claim otherwise.

A lot of these maintainers are already overworked and don't have time to tutor every single person that wants to put kernel contributor on their resume. A lot of people have this extremely idealistic view of open source but the reality is that the vast majority of patches you receive will be atrocious and you shouldn't have to tutor every single "potential" contributor. This case is even worse since this issue was posted to a security mailing list which means time to resolution is important.

Who knows how long this would have been unaddressed without Ariel Miculas initial input.

Part of the job of a maintainer is to curate and foster input so the project has a robust community in the future.

The maintainer in this case did a miserable job of that.

On the other hand, to dig yourself out of the hole of “I’m too busy and there’s nobody around to help”, you have to slow down and do this, sometimes.

> to dig yourself out of the hole of “I’m too busy and there’s nobody around to help”, you have to slow down and do this

Exactly! If you treat new contributors as crap because you are overworked, you will push away potential long-term collaborators who could share the load thus ensuring you will be working alone far into the future. This is a failure state and if I had a manager doing that to those he was managing, he wouldn't be a manager for long.

> A lot of these maintainers are already overworked and don't have time to tutor every single person that wants to put kernel contributor on their resume.

This seems like a very cynical view of what happened here, and completely unfounded.

Obviously these big open-source projects need gatekeepers, but if the gatekeepers often make aspiring contributors feel violated and humiliated rather than offering feedback to get their initial contributions across the finish line, general interest and enthusiasm for contributing are going to dry up. Maybe that's by design, but it doesn't feel sustainable in the long term.

> feel violated and humiliated

A lot of contributors are going to feel like this if their patch doesn't get accepted immediately and the maintainer in this case was extremely respectful even though the offered patch had multiple major issues. Contributors shouldn't feel entitled to free tutoring.

> multiple major issues

Seems debatable and debated, reading through the rest of this thread.

Regardless, I'll reiterate my point more straightforwardly: it's the maintainers' prerogative to treat aspiring contributors however they like, but actions have consequences, and some of those consequences may be deleterious to a project's health.

> Seems debatable and debated, reading through the rest of this thread.

Most of the people commenting in this thread haven't looked at the code and anyone saying the two patches are equivalent can be safely ignored. Out of the issues kazinator pointed out, 1 and 2 are definitely major issues and 3 is debatable although it is definitely suboptimal.

> some of those consequences may be deleterious to a project's health

Of course, but I think it might not be in the way the "everyone can be a valued contributor" crowd thinks.

> the "everyone can be a valued contributor" crowd

Of course! Who will keep the hordes of dimwits and dilettantes and LinkedIn jockeys at bay, if not those whose merit has already been proven?

> Who will keep the hordes of dimwits and dilettantes and LinkedIn jockeys at bay

This is unironically a very serious problem and why most of the best open source projects are basically corporate FAANG projects that are really only open source for optics.

On its face, you won't catch me arguing with that. It's a hard problem, for sure.

See https://news.ycombinator.com/item?id=37692883

It would be quite sad if we've gotten to a point where saying "I think my patch is better than yours" from a domain expert to a newbie is considered humiliating and violating.

Talking about utilitarian consequences, Linus had been way worse and while I fully agree from a moral perspective that his most extreme behaviors were bad, from a purely utilitarian perspective that didn't prevent Linux from being one of the most successful OSS projects out there.

> It would be quite sad if we've gotten to a point where saying "I think my patch is better than yours" from a domain expert to a newbie is considered humiliating and violating.

Isn't this a little bit disingenuous? Do you really think the author of OP's blog post is upset simply because a Linux kernel maintainer has far more domain expertise than him and is in principle capable of writing a "better" patch?

That would be pretty irrational, I agree!

The real disingenuous part is where OP lied about the maintainer saying his patch was better.

It would be pretty bad if he said that, but he never did.

I agree that Miculas (the submitter of the original patch) should not have chosen formatting typically associated with direct quotations, but now you're the one putting words in his mouth: he didn't say Ellerman said his (Ellerman's) version was better, he (Miculas) said Ellerman said he liked his (Ellerman's) version better.

Ellerman's actual words were "I wanted to fix it differently", so while I don't think Miculas' paraphrasing was entirely appropriate (why paraphrase at all?), I also don't think it was a misrepresentation of what Ellerman actually said.

Because it could be seen as communicating: "my ten minutes of coding around the array access problem is more perfect than your hours of investigation to root cause the issue plus your ten minutes of coding".

Besides the fact that he didn't actually say that at all.

> This seems like a very cynical view of what happened here, and completely unfounded.

It is not unfounded at all. The author clearly stated the reason they wanted their commit to be accepted was should they could be contributor.

Do you think there is any reason somebody might want to be credited for work they did other than having the ability to put it on their resume?

He had the credit amongst those who were paying attention. It wasn't like this was done in secret. Others chimed in to agree that the maintainers commit was better. So the only credit he didn't get was a commit credit which is important to be a "Linux Contributor" which clearly explictly wrote was the reason they asked for their commit to be used or to work on another patch.

I think it would be odd to disbelieve the author.

> Maybe that's by design, but it doesn't feel sustainable in the long term.

That could be, but on the other hand, the Linux kernel is more than 30 years old and is on its 6th version...

One might also consider the rarity of maintainers who can write high quality code and donate large amounts of time. To find and retain someone like that one might also be required to allow them to be curt with random people on the internet from time to time. I can see how drive by contributions could get old fast from their point of view.

> That could be, but on the other hand, the Linux kernel is more than 30 years old and is on its 6th version...

Indeed, and while we're speaking in broad generalities, surely no strategy that worked to launch a project from tiny seed to maturity and market saturation has ever then failed to keep it relevant in the long term...

> I can see how drive by contributions could get old fast from their point of view.

Indeed. But (again addressing generalities) my issue here is that the contribution in question doesn't read as "drive by" to me.

> surely no strategy that worked to launch a project from tiny seed to maturity and market saturation has ever then failed to keep it relevant in the long term...

If the rate of submissions falls then it certainly makes sense to adjust. Until then, don't fix what isn't broken.

Frankly, if you treat effortful but green contributors the same way you treat grindset morons and LinkedIn jockeys, I would put money on the submission rate from the latter continuing to increase long after most of the former have decided your project isn't worth their time.

This is a bit of a moot point, though, since I don't think it's very common for Linux kernel maintainers to behave this way. Correct me if I'm wrong.

I looked at the two patches, and TBH the time taken to write the final correct patch would be a fraction of the time needed to explain how to write a better patch.

In a company where people are expected to stay around for a while, it may make sense to invest time to bring juniors up to speed. But I wonder whether that's true for OSS projects in general. A person randomly shows up to report and attempt fix a bug in an "obscure"(?) architecture -- this is not a junior who just joined the team, and they have no commitment to stick around to do more contributions.

In this case, from a utilitarian perspective, it doesn't seem obvious the maintainer should spend the extra time coaching.

This might be a minority view here, but TBH I don't think anyone should be obliged to go out of their way to spend extra time and effort to avoid offending a random person on the Internet.

The time to write the patch is insignificant in this context.

I would say that the author stated he asked for his patch to be accepted so he could be a contributor implies they weren't planning on contributing more. So it doesn't seem like a sceranio where they should spend that extra time.

Not really.

1. Please don't drop cases from your #if CONFIG_PPC32 blocks that are handled on 64 bit. You can use if (IS_ENABLED(CONFIG_PPC32)) instead of #ifdef so the only difference is in the array access.

2. Please remove the FPRINDEX macro; as you note in your commit message, we can assume TS_FPRWIDTH is 1.

The maintainer also wrote a long and detailed commit message in his own version of the patch; he had time to explain things.

In this exact situation, I would possibly have made the code adjustments myself and written "patch provided by Ariel Miculas". Then send an e-mail saying that I made a few minor tweaks to your patch, here is what it looks like.

"patch provided by" tells the truth: that person provided a working patch. It doesn't say that the patch they provided is precisely this one.

A maintainer can't approve a patch that wasn't properly signed off, that's one of the very few rules set for patch submission that has nothing to do with the code but everything to do with the legal implications of including code that isn't properly attributed and an essential part in keeping the Linux kernel unencumbered.

This is not some rubberstamp formality, it is an actual declaration of authorship and shields the kernel maintainers from possible claims of copyright infringement.

> TBH the time taken to write the final correct patch would be a fraction of the time needed to explain how to write a better patch.

This is always the case for a senior developer dealing with a junior developer. Basically your argument is that junior developers shouldn't try to contribute and if they do, they will just be rejected out of hand if they make any mistakes. Okay.

> But I wonder whether that's true for OSS projects in general. A person randomly shows up to report and attempt fix a bug in an "obscure"(?) architecture -- this is not a junior who just joined the team, and they have no commitment to stick around to do more contributions.

If you treat people like crap, they won't stick around. So it is a bit of a self-fulfilling prophecy.

I think that great OSS leaders are few and far between. One that I can think of that I've worked closely with for a decade is mrdoob of Three.js. Humility, openness, focus and good people skills. I cannot approach his skill set here, and in part that is why I respect it so much. He achieves a lot and there are hundreds of contributors to Three.js of all different skill levels. And he definitely is not going around rewriting simple PRs just because he can and instead he and others are coaching and encouraging the community. This is why Three.js thrives.

I think this particular OSS developer isn't leader and is more of an individual contributor type who doesn't collaborate well. That is okay. It is unfortunate that his behavior is reflecting poorly on the Linux project, but that is who he is.

> Basically your argument is that junior developers shouldn't try to contribute and if they do, they will just be rejected out of hand if they make any mistakes. Okay.

I don't think that's as wrong as it sounds. There are projects and codebases that are more or less "done" (and heading towards obsolescence) and aren't worth it to expend resources for juniors to learn how to work on them.

It's usually much more rewarding to have junior developers try to implement something "new" instead of fixing obscure bugs on a stable codebase. There's much more leeway for mistakes in new features, there is less/no learning curve to grok the intricacies of the existing code, and fewer surprising edge cases that the juniors would inevitably miss because they haven't been stung by it 10 years ago.

As I mentioned, it's up to the projects themselves to decide how much they want to expend extra resources in bringing up potential new contributors. As with most things in software engineering, it's a trade off.

And yes, the OP feeling angry with the way the kernel maintainer handled this issue could be said to be a trade off as well. I think the only thing that's unfair about this discussion is that you and maybe a hundred others are making claims about the kernel maintainer ("isn't leader", "behavior reflecting poorly on the Linux project", "treat people like crap") for allegedly misattributing credit for a less than 20 line patch.

Imagine a minor mistake you made 1.5 years ago making the front page of HN and hundreds of people slinging insults at you. I wonder how that feels. Quoting from you:

> If you treat people like crap, they won't stick around.

> I think that great OSS leaders are few and far between.

We can also revert this and say that great contributors are "few and far between".

Collaboration, patience, and empathy is a two-way street. Could the maintainer have done better? I guess; I would have done things a bit different. Then again, maybe their dog died that morning and they were in a foul mood? I'm not joking: people aren't perfect, they have good and bad days for all sorts of reasons, etc. Everyone makes a mistake now and again.

But whatever this maintainer could have done better, this blog post also isn't a good look: sour grapes a year and a half later and making a big deal out of what is really a singular minor personal conflict (and with some misrepresentations to boot). Now, maybe the author's dog died this morning, so I'm not going to attach any far-fetched conclusions about this person, or about Cisco, because again, everyone has bad days and no one is perfect.

Making a big deal out of any mistake or conflict is also toxic, as is making rather strong conclusions about a person based on a single event.

> But whatever this maintainer could have done better, this blog post also isn't a good look

You are correct, I definitely would not have written that blog post either.

> Making a big deal out of any mistake or conflict is also toxic, as is making rather strong conclusions about a person based on a single event.

I believe what you are describing is formally called the Central Attribution Error (CAE.) https://ethicsunwrapped.utexas.edu/glossary/fundamental-attr...

You are right, I should edit my original comment to remove my speculations that are probably based on CAE but it is now locked for editing.

You're missing the point. "I can do it faster" discourages new contributors from wanting to help out ever again.

> a fraction of the time needed to explain how to write a better patch

He apparently had six years (since bug was reported) to do it. I think he surely could spend some time.

>A person randomly shows up to report and attempt fix a bug in an "obscure"(?) architecture

Everyone involved in this story, both the original author, the second person who authored the patch, and the maintainer who reviewed it, are on the mailing list for that obscure architecture. It's obvious that they all care about it. Its "obscurity" in your eyes is irrelevant.

>In this case, from a utilitarian perspective, it doesn't seem obvious the maintainer should spend the extra time coaching.

https://news.ycombinator.com/item?id=37675328

> https://news.ycombinator.com/item?id=37675328

I was replying to a specific comment suggesting that the maintainer should do a code review, give comments and ask for an update to the patch, instead of rewriting the patch. Quoting my words out of context isn't helpful here.

And my point was that from "a utilitarian perspective" there's a middle ground between "coach the original author to produce a better patch" and "commit your own patch without crediting the original author"; it's "commit your own patch while crediting the original author". There's no "out of context" quoting in my comment.

What usually happens is that someone sends a patch with a text description of the problem, but no test case. So what I do in those cases is just quickly look at the patch and create a test case first, on the current stable version, to reproduce the bug. This is both so that a test case exists, but also so that I have a deeper understanding of the problem.

When that is done, often the fix is simple, and the author's patch sometimes just ... isn't that great, often for entirely understandable reasons, so it's easier to just fix it myself "from scratch".

So whose patch is it then? In some cases the patch was useful and I'm "inspired" by it, in other cases I'm not. I tend to err on the side of caution, but I've definitely merged my share of patches where the Author field was, IMHO, kind of a lie.

As for code review: it's really time-consuming. And while a few "minor bug fixers" of today become "hard-core contributors" or tomorrow, realistically, most people show up to fix one bug and are never seen again. That's perfectly fine! But it does mean that "teaching" everyone who shows up quickly become an enormous time sink. There's a bit of a trade-off to be made here.

"This change is closely based on a well-considered patch from A. U. Thor <author@example.com>, who also originated the bug report."

A. U. Thor can point to that 10 years later and say he or she fixed that; nobody cares if the maintainer changed a few things.

Debugging is hard, sometimes very hard. Writing a fix on a familiar code base is comparatively trivial, especially having a working version of the prior fix as the guide. Writing the second version is always easier.

Saying that it’s a simple fix shows one doesn’t understanding the amount of work go into bug fixings in software development.

In this case Michael even acknowledged that he could not reproduce it initially but only later had it nailed down. This speaks volume about the amount of work OP put into debugging and fixing the problem. Robbing him of his work is just not right.

If you've been chasing a bug for a while and finally have a fix, you may have code blindness to ever-important issues not related to the fix. The testing you're doing is whether or not that problem went away. This is true even if you're familiar with the code base.

That's why we have pair programming and code reviews.

That same developer who spotted your code blindness today will exhibit that themself tomorrow.

The person not involved in the feature or fix is completely free to think about nothing else but "in what ways is this change wrong"?

>Sorry, just because you found this trivially fixable array mismatch doesn't mean your buggy patch has to be accepted the way it is.

I know reading is hard, but it's not an excuse to put words in other people's mouths.

>>I told him that I would really appreciate if he could accept a patch from me, so that I could receive credit for fixing this issue and become a kernel contributor. I was also open to working with him, addressing his feedback and sending subsequent versions of patches.

So the maintainer now has to tutor this guy for god knows how long until he gets it right? All three issues brought up by kazinator are reasonable grounds for ignoring the patch. Do you think that the already overworked maintainers should have to spend significant amounts of their time tutoring every potential contributor? Are you going to pay them for that?

It doesn't sound like you've ever maintained an active free software project.

It's not "tutoring" to perform a code review and request additional changes to the patch. That's a standard feedback mechanism in free software development.

kazinator's analysis may be right, but it doesn't sound like it would have taken the maintainer more than a few minutes to explain his objections and given the contributor a chance to shore up his patch.

There's plenty of examples in the history of the kernel project of much larger patches going round and round until they land. From kazinator's description, it's possible the patch could have been made ready in a single review round.

> All three issues brought up by kazinator are reasonable grounds for ignoring the patch.

The patch (and bug report) wasn't ignored. That's kind of the point.

Miculas is not dumb; he's obviously a self-starter who pulled the investigation together through gdb, and the kernel interface, plus surrounding research into the history of the problem. He got excited to close the array access problem and lost sight of the necessity to fix only that and not change anything else, like the bounds check on the index. All it needed was a nudge to put his attention to that detail.

He could just have asked as a review of the the maintainers patch[1], e.g. work with the maintaner to fix it.

[1] https://lore.kernel.org/all/20220609133245.573565-1-mpe@elle...

I honestly don't know how two people could "work together to fix it" for an apparently straightforward patch where less than 20 lines are changed...

> I told him that..

Is this true though?

> Sorry, just because you found this trivially fixable array mismatch doesn't mean your buggy patch has to be accepted the way it is.

That's not really what anyone is suggesting should have happened. The maintainer should've just taken the two minutes of extra time required to checkout the OP's code, make whatever changes the maintainer wanted and commit it with co-authorship.

That clearly would've been the best course of action for the maintainer to follow (especially given that the OP expressed his desire to the recognized for his contribution) -- the best fix gets merged in and the work of others is acknowledged/respected. That said, I'm not surprised it didn't happen -- social niceties often seem to escape the maintainers of these sorts of repos...

I've written countless of patches / PRs for diverse OSS . Most of the times was to scratch an itch.

Most of the time as well the PR doesn't get accepted because of various reasons. I don't care. I do the PRs so that the code is out there. No I wont spent time making it "good enough " to merge into your repo. I provide it as is, and I'd someone likes it enough they can do the red tape.

Some of it has been just accepted and merged. Some was slightly modified by the repo owner before merging. Good for them.

I dont do open source code for the glory or to be popular, I do it because I like it .

Aaah I miss the 80s and 90s.

1. As I said in the original thread:

  There is already a check at the beginning of the function:
  if (index > PT_FPSCR)
     return -EIO;

2. fpscr will actually be written to by this line of code:

  +       ((unsigned int \*)child->thread.fp_state.fpr)
  +               [FPRINDEX(index)] = data;

3. This is a nitpick.

So please tell me how my patch was buggy.

1. The index > PT_FPSCR test isn't good enough:

  int ptrace_get_fpr(struct task_struct *child, int index, unsigned long *data)
  {
          unsigned int fpidx = index - PT_FPR0;

          if (index > PT_FPSCR)
                  return -EIO;

          flush_fp_to_thread(child);
          if (fpidx < (PT_FPSCR - PT_FPR0))
                  memcpy(data, &child->thread.TS_FPR(fpidx), sizeof(long));
          else
                  *data = child->thread.fp_state.fpscr;

          return 0;
  }

It is testing the original signed value of type int, which could be negative, and therefore convert to a huge unsigned int value manifested in fpidx.

(Also, the function risks integer subtraction underflow in "index - PT_FPR0". If we pass in the value INT_MIN, subtracting a positive value from it is undefined behavior.)

It looks like the original author knew that the -EIO test is too weak, so the converted unsigned value is tested again.

Regarding 2, accessing fpscr using fpr[32] is undefined behavior (out of bounds array access), which is probably why the original code is refraining from that, but testing for that value and separately accessing the member.

However is also this bit of uncertainty (I have not fully drilled into). On 32 bit PPC, that PT_FPSCR is defined with an extra offset of 1:

   #define PT_FPSCR (PT_FPR0 + 2*32 + 1)

I suspect that this is due to PPC being big endian; the + 1 displaces the index to the lower half of a 64 bit word. This also tells me that this fpscr register must be 32 bits on PPC32?

Thus, even with the range check in place, out-of-bounds access past the array is possible if user space requests the [64] index since the range check is testing for below 65. The [64] index isn't fpscr, but the upper half of u64 fpscr.

There is also the aspect that when we assign to fpscr, the whole 64 bit value is assigned, not just half of it.

The correct thing to do is to leave these comments as a code review and let the contributor fix it. No one argued in favor of accepting a buggy patch.

Lmao, even GH does something where it's: "by ORIGINAL_AUTHOR, co-authored by MAINTAINER". Ah but the kernel is a special secret club indeed.

Reply on news.ycombinator.com