Mein erster Kernel-Beitrag wird mir weggenommen

• Ariel Miculas verfolgte ein Problem, bei dem Multithread-Debugging mit gdbserver auf PowerPC32 kaputt war, fand dabei einen Speicherfehler im Linux-Kernel, erhielt am Ende aber nur eine Reported-by-Nennung, weil der endgültige Fix als separates Patch des Maintainers einging
• Hinweise waren ein älterer Mail-Thread mit denselben Symptomen sowie ein Commit, der die Position von thread_struct thread innerhalb von task_struct veränderte; dadurch blieben einige Threads in einem falschen Prozesszustand hängen und gdbserver verlor die Kontrolle
• Für die Ursachenanalyse kamen pahole, ftrace und hardwarebasierte Breakpoints auf Basis des PowerPC-DABR zum Einsatz; dabei wurde bestätigt, dass ein Buffer Overflow in ptrace_put_fpr Felder wie task_struct.__state überschreibt
• Der Overflow entstand, weil ein Index für 32-Bit-Elemente auf ein 64-Bit-Array angewendet wurde; dadurch konnte gdbserver auf der User-Seite hinter das nur 32 Einträge große fp_state.fpr-Array schreiben, und zwar um bis zu 256 Byte
• Der PowerPC-Maintainer Michael Ellerman übernahm einen eigenen Fix; in einer späteren Antwort folgte eine Entschuldigung, dass Ariels Beitrag durch ein bloßes Reported-by nicht angemessen gewürdigt wurde und man das Patch gemeinsam hätte weiterentwickeln sollen

---

Kaputtes gdbserver-Debugging auf PowerPC32

• Vor etwa anderthalb Jahren bat Ariel Miculas in seiner früheren Firma darum, Zeit für die Untersuchung eines Problems zu bekommen, das die Debugging-Funktionen eines Projekts blockierte
• Das Problem bestand darin, dass gdbserver auf der PowerPC32-Architektur Multithread-Anwendungen nicht korrekt debuggen konnte
  • Die gdbserver-Verbindung brach ab, und die Debug-Session ließ sich danach nicht mehr kontrollieren
  • Es war unklar, ob die Ursache im Toolchain, in gdbserver, im Linux-Kernel oder in kundenspezifischen Kernel-Patches lag
• Nach bestehender Recherche und weiterer Suche fand er einen Mail-Thread, der dieselben Symptome behandelte
  • Dieser Thread verwies auch auf den Commit, der das Problem ausgelöst hatte
  • Das betreffende Patch verschob die Definition thread_struct thread, die zuvor in der Mitte von task_struct stand, ans Ende

Hinweise, die auf Speicherbeschädigung im Thread-Zustand hindeuteten

• Laut einer früheren Analyse von Holger Brunck schickte gdbserver dem Kernel für jeden Thread ein SIGSTOP und wartete auf eine Antwort; im Fehlerfall antwortete der Kernel jedoch nur auf einen Teil der Signale
• Auf niedriger Ebene landeten einige Threads nach der Interaktion mit gdbserver in einem falschen Prozesszustand, woraufhin gdbserver die Kontrolle verlor
• Ariel las drei bis vier Tage lang Beschreibungen von PowerPC-bezogenen Commits und Änderungen rund um task_struct, um zu prüfen, ob das Problem in späteren Kernel-Versionen behoben worden war, fand aber keine Lösung
• Er veränderte die Position von thread_struct thread, um die Reproduktionsbedingungen zu prüfen, und untersuchte mit pahole das Layout von task_struct
• Mit ftrace verfolgte er, wann die Threads des zu debuggenden Prozesses eingeplant wurden
  • Die hängengebliebenen Threads wurden im Unterschied zu den anderen nur ein einziges Mal eingeplant
  • Diese Beobachtung lenkte den Verdacht wieder auf Speicherbeschädigung

Die eigentliche Ursache, gefunden mit Hardware-Breakpoints

• Er stellte fest, dass PowerPC ähnlich wie die Debug-Register auf x86 eine vergleichbare Funktion über das DABR-Register bietet
• Nachdem er recherchiert hatte, wie man unter Linux Hardware-Breakpoints verwendet, implementierte er auf Basis einer Stack-Overflow-Antwort ein Kernel-Modul
• Mit diesem Modul setzte er einen Hardware-Breakpoint auf das Feld task_struct.__state, um nachzuverfolgen, wer den Wert schreibt
• Das benutzerdefinierte Kernel-Modul zeigte den Stack-Trace der Stellen, die auf task_struct.__state schrieben; darunter führte ein verdächtiger Pfad zu einem Buffer Overflow in ptrace_put_fpr
• ptrace_put_fpr wird in der POKEUSER-API verwendet, und der Overflow überschreibt wichtige Felder von task_struct wie __state
  • __state speichert den Prozesszustand
  • Der Kernel nutzt dieses Feld auch, wenn er von einem Debugger angehaltene Prozesse nachverfolgt

Array-Schreibzugriffe konnten bis zu 256 Byte über das Ende hinausgehen

• Die Ursache des Overflows war, dass ein Index, der für ein Array aus 32-Bit-Elementen gedacht war, auf ein 64-Bit-Element-Array angewendet wurde
• Der Index auf die FPR umfasste 64 Werte, und der Adressbereich bei 64-Bit-Elementen betrug 64 * 8 = 512 Byte
• Das tatsächliche fp_state.fpr-Array hatte jedoch nur 32 Einträge, also nutzbaren Speicher von 32 * 8 = 256 Byte
• Dadurch konnte der User, also gdbserver, hinter das Ende des Arrays schreiben, und zwar um bis zu 256 Byte

Upstream-Einreichung und Streit um die Anerkennung

• Weil Ariel davon ausging, dass das Problem der Speicherbeschädigung mit dem Überschreiben des Prozesszustands sicherheitsrelevant sein könnte, schickte er ein Patch an [e-mail]
  • Diese Mailingliste ist privat, deshalb konnte das ursprünglich eingereichte Patch nicht verlinkt werden
• Der PowerPC-Maintainer Michael Ellerman meldete sich privat und antwortete, dass er sich um das Problem kümmern werde
• Ariel schickte zwei Fix-Patches
  • das ursprüngliche Patch an die Sicherheits-Mailingliste
  • eine andere Patch-Version, die Vorschläge zum ursprünglichen Einreichen berücksichtigte
• Das zweite Patch basierte auf vorhandenem Kernel-Code, der auf PowerPC64 das Verhalten von PowerPC32 emulierte, und dieser Code behandelte das FPR-Indexing bereits korrekt
• Michael Ellerman akzeptierte keines der beiden Patches und implementierte stattdessen einen eigenen Fix
• Ariel wollte, dass sein Patch akzeptiert wird, sowohl wegen der Anerkennung für die Behebung des Problems als auch als Würdigung seines ersten Kernel-Beitrags; er war auch bereit, eine überarbeitete Version nach Feedback einzureichen
• Tatsächlich erhielt er nur eine Reported-by-Kennzeichnung, was Ariel im Verhältnis zu seinem Aufwand für Ursachenanalyse, Fix, Tests, Verifikation, internes Feedback und die Anpassung an aktuelle Kernel-Versionen als unfair empfand
  • Laut Linux-Dokumentation dient Reported-by dazu, die Person zu nennen, die einen Bug gefunden und gemeldet hat
  • Das betreffende Problem war allerdings bereits sechs Jahre zuvor gemeldet worden

Update und Entschuldigung per Antwortmail

• Ein Hacker-News-Nutzer wies auf eine Mail hin, in der Michael Ellerman schrieb: „Danke für das Patch, aber ich möchte es anders beheben. Kannst du bitte das untenstehende Patch testen und prüfen, ob der Bug damit behoben ist?“
• Diese Mail zeigt, dass Michael Ellerman statt einer Review von Ariels Patch eine eigene Implementierung bevorzugte und Ariel bat, dieses Patch zu testen
• Ariel suchte das ursprünglich an die Sicherheits-Mailingliste geschickte Patch heraus und leitete den Mail-Thread an linuxppc-dev weiter
• Ein späteres Update enthielt dann eine Entschuldigung für die Art, wie mit dem Patch umgegangen wurde
  • Darin hieß es unter anderem, man hätte mehr Zeit darauf verwenden sollen, sein Patch gemeinsam weiterzuentwickeln
  • Außerdem wurde eingeräumt, dass ein Reported-by-Tag Ariels Beitrag nicht angemessen widerspiegelte

[GN⁺]

Hacker-News-Meinungen

• Auch wenn man den gesamten Patch nicht akzeptiert, wäre es angemessener gewesen, Ariel Miculas die Anerkennung zu geben.
  Dieses Sicherheitsproblem wäre sehr wahrscheinlich von niemandem behoben worden, wenn er es nicht gefunden und einen Fix eingeschickt hätte.
  Wenn Michael seinen Patch gelesen, nur Teile des Stils geändert und ihn dann unter eigenem Namen eingereicht hat, ist es unethisch, keine Anerkennung zu geben. Selbst wenn der Code schlecht gewesen wäre, war es immer noch Zusammenarbeit; wenn einem so etwas passiert, möchte man danach nicht noch einmal zusammenarbeiten.

  • Früher dachte ich, es sei in Ordnung, Beiträge anderer zu übernehmen, sie in meinem Branch komplett neu zu schreiben und zu mergen oder sie in einem GitHub-PR als neuen Commit neu einzubauen.
    Bei Letzterem bleibt immerhin Co-Authored-By erhalten, aber erst als jemand Code aus meinem Projekt ohne Commit-Historie in sein eigenes Repository kippte und es als „Fork“ bezeichnete, wurde mir klar, dass das aus Sicht eines Beitragenden eine ziemlich bittere Erfahrung ist.
    Auch wenn der Code schlecht war, hätte der gemergte, refaktorierte Fix ohne diesen Code nicht existiert, und „meine Version ist besser“ kann sich ziemlich herablassend anfühlen.
    Allerdings gilt der letzte Satz nicht, wenn man die Antwort mit dem Link zur tatsächlichen Antwort des Maintainers liest.
  • Stimme zu. Ein Maintainer kann wegen der Qualität sagen, dass er den Code, der in den Kernel kommt, selbst schreiben will, und das ist an sich nachvollziehbar.
    Aber in diesem Fall ist der Autor des Beitrags eher ein Co-Autor dieses Patches.
    In der wissenschaftlichen Publikation wäre das ungefähr so, als würde man jemandem ein Preprint schicken, und diese Person veröffentlicht dann ein Paper zum selben Thema, nennt einen nur in der Danksagung, aber nicht als Co-Autor.
    Hier geht es nicht um Publizieren, sondern um Kernel-Entwicklung, aber die Autorennennung ist im beruflichen Lebenslauf trotzdem wichtig.
  • Ich sehe die Rolle erfahrener Maintainer als Unterstützer, die so gut wie möglich dabei helfen, dass Beiträge anderer sauber einfließen.
    Feedback zu Stil und struktureller Konsistenz ist nötig, aber ein guter Maintainer erkennt Leistungen an und sorgt im Hintergrund dafür, dass die Dinge reibungslos laufen.
  • Als ich meine ersten paar Patches an den Linux-Kernel schickte, war noch nicht einmal die halbautomatische Einreichung über LKML möglich, also lief es im Gespräch mit dem Maintainer.
    Damals war Ted Tso mit der Korrektheit meiner Arbeit zufrieden und nahm den Patch auf; ich verlangte weder Anerkennung noch bekam ich welche.
    Mir ging es nur darum, den Kernel zu verbessern, und nicht darum, meinen Namen zu hinterlassen.
    Ein paar Jahre später landete mein Name wegen anderer Arbeit zwar in der Contributor-Liste, aber so oder so war mir das egal.
  • Er hat Anerkennung bekommen. Im Commit steht Reported-by: Ariel Miculas, und das scheint korrekt zu sein.

• Der Patch des Maintainers ist besser.

  1. Er verliert im 32-Bit-Fall nicht die Prüfung fpidx < (PT_FPSCR - PT_FPR0).
  2. Er verliert nicht die Ersatzzuweisung *data = child->thread.fp_state.fpscr; in der put-Funktion, die verwendet wird, wenn fpidx außerhalb des Bereichs liegt.
  3. Er vermeidet das unnötige Makro FPRINDEX, das sich zur Identität reduziert, wenn TS_FPRWIDTH 1 ist.
     In der Commit-Message des ursprünglichen Patches stand: „Für PPC32 kann man davon ausgehen, dass TS_FPRWIDTH 1 ist“, trotzdem enthielt er FPRNUMBER, FPRHALF und FPRINDEX.
     Nur weil jemand eine Array-Diskrepanz gefunden hat, die sich einfach beheben lässt, heißt das nicht, dass man einen fehlerhaften Patch unverändert übernehmen muss.
     Allerdings hätte der Maintainer anerkennen sollen, dass Miculas nicht nur etwas gemeldet hat, sondern es untersucht, die Root Cause gefunden und sogar eine eigene Version eines fast identischen einfachen Fixes erstellt hatte.
     Reported-by ist ungenau, wenn es die Nuance transportieren soll, dass die betreffende Person gründlich untersucht und die Ursache ermittelt hat.
  • Ich stimme zu, dass die Arbeit des Maintainers besser ist. Aber man hätte einfach ein Code-Review zum Patch machen und ein Update anfordern können.
    Bei neuen Beitragenden macht man das in Projekten häufig so, und das ist der richtige Weg.
    Es ist ein Prozess, Menschen anzuleiten und einzuarbeiten; wenn man die Arbeit einfach neu macht, nimmt man ihnen die Motivation beizutragen und auch die Chance zu lernen.
  • Debugging ist schwer, manchmal sehr schwer. Einen Fix in einer vertrauten Codebasis zu schreiben, ist vergleichsweise trivial, vor allem wenn man eine funktionierende Version eines vorherigen Fixes als Leitfaden hat.
    Die zweite Version zu schreiben ist immer einfacher.
    Das als simplen Fix zu bezeichnen, heißt, den Arbeitsaufwand für Bugfixes in der Softwareentwicklung nicht zu verstehen.
    Auch Michael gab hier zu, dass er es zunächst nicht reproduzieren konnte und es erst später bestätigte.
    Das zeigt gut, wie viel Arbeit OP in Debugging und Fix gesteckt hat, und es ist nicht richtig, diese Arbeit wegzunehmen.
  • Die Aussage „Nur weil jemand eine Array-Diskrepanz gefunden hat, die sich einfach beheben lässt, heißt das nicht, dass man einen fehlerhaften Patch unverändert übernehmen muss“ legt anderen Worte in den Mund.
    Im Original hieß es, dass er wollte, dass der Patch angenommen wird, er die Anerkennung bekommt und Kernel-Contributor wird, und dass er bereit gewesen wäre, eine Folgeversion mit eingearbeitetem Feedback zu schicken.
  • Niemand sagt, dass der ursprüngliche Patch unverändert übernommen werden musste.
    Der Maintainer hätte OPs Code auschecken, die gewünschten Änderungen vornehmen und die etwa 2 Minuten investieren können, die nötig sind, um den Commit als Co-Autor zu erstellen.
    Dann wäre der beste Fix gemergt worden und die Arbeit einer anderen Person wäre ebenfalls anerkannt worden.
    Besonders weil OP klar geäußert hatte, dass er Anerkennung bekommen möchte, wäre das der beste Weg gewesen.
    Allerdings scheinen Maintainer solcher Repositories soziale Höflichkeit oft zu übersehen, daher überrascht es mich nicht.
  • Ich habe viele Patches und PRs an verschiedene Open-Source-Projekte geschickt, meistens, um eigene Unannehmlichkeiten zu beheben.
    PRs werden aus vielen Gründen oft nicht angenommen, aber das stört mich nicht.
    Ich schicke PRs, um den Code öffentlich verfügbar zu machen, und investiere keine Zeit, ihn „gut“ genug für einen Merge zu machen.
    Ich stelle ihn einfach bereit, und wenn er jemandem ausreichend gefällt, kann diese Person den Prozess übernehmen.
    Manches wurde unverändert gemergt, anderes hat der Repository-Owner leicht angepasst und dann gemergt.
    Open-Source-Code schreibt man nicht wegen Ruhm oder Popularität, sondern weil man es gern tut.
    Ich vermisse die 80er und 90er.

• Der Linux-Kernel ist keine Belohnung, und auch wenn ich Mitgefühl mit OP habe, ist es richtig, dass die Community eine besser wartbare Lösung einer von außen kommenden Version vorzieht.
  Insofern lag der Maintainer innerhalb seines Zuständigkeitsbereichs.
  Trotzdem ist es sehr einfach, der Person, die zur Korrektur beigetragen hat, Anerkennung zu geben.
  Bei Node.js wird in den Release Notes Anerkennung gegeben, und wenn wir Teile der Arbeit aus einem PR übernehmen und anpassen, versuchen wir, Co-Authored-By: hinzuzufügen.
  Wenn man lange maintaint, wird Anerkennung für einen selbst weniger wichtig; dadurch vergisst man leicht, dass sie für neue Beitragende sehr wichtig ist.
  In einer Situation wie dieser, in der das Teilen der Anerkennung für OP wichtig und auch angemessen gewesen wäre, war die fehlende Sorgfalt ein Verlust sowohl für OP als auch für den betreffenden Linux-Bereich.

  • Wenn man viel Mühe in ein Projekt steckt und einen Beitrag überarbeitet, nur um am Ende zu hören: „Meine Version ist besser, also geh weg“, wäre man verärgert.
    So lässt sich ein Projekt, das auf Community-Arbeit angewiesen ist, nicht dauerhaft betreiben.
    Bei einem so großen und angesehenen Projekt wie dem Linux-Kernel hätte ich erwartet, dass es Leute gibt, die sowohl das Beste für das Projekt im Blick haben als auch neue Beitragende respektieren können.
    Niemand verlangt, eine nicht optimale Lösung nur deshalb anzunehmen, weil sie von einem neuen Beitragenden kommt.
    Der richtige Weg ist, den Patch zu reviewen, zu erklären, was warum problematisch ist, vorzuschlagen, wie man ihn besser umschreiben könnte, und diesen Teil bei Bedarf gemeinsam zu schreiben und dadurch Co-Autor zu werden.
    Nur so entsteht auch künftig ein Strom interessierter Beitragender.
  • „Der Linux-Kernel ist keine Belohnung“ scheint in Bezug auf diesen Artikel im schlechtesten Sinne zu stimmen.
  • Es sieht so aus, als hätte er mit dem Tag Reported-By Anerkennung erhalten.

• In der Kernel-Dokumentation ist Suggested-by als eine der Tag-Konventionen definiert.
  Das Tag Suggested-by: zeigt an, dass die betreffende Person die Idee für den Patch vorgeschlagen hat, und stellt sicher, dass ihr für diese Idee Anerkennung gegeben wird.
  Insbesondere heißt es dort, dass es nicht ohne Erlaubnis der vorschlagenden Person hinzugefügt werden sollte, wenn die Idee nicht in einem öffentlichen Forum gepostet wurde.
  Außerdem steht dort sinngemäß, dass eine gewissenhafte Anerkennung der Ideengeber die Wahrscheinlichkeit erhöht, dass sie auch in Zukunft wieder helfen.
  In dieser Situation wäre Suggested-by möglicherweise passender gewesen.
  Es vermittelt besser, dass die Person die Lösung für das Problem gefunden hat, aber mit dem Projekt nicht vertraut genug war, sodass die genaue Syntax des Patches nicht beibehalten wurde.
  Referenz: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/lin...

  • Ich habe die Patches nicht verglichen, aber laut dem Kommentar unten [1] bestand der einzige Unterschied zwischen dem akzeptierten und dem vorgeschlagenen Patch in der Stelle der Änderung.
    Der eine befindet sich in einem ifdef außerhalb des if, der andere im inneren if-Statement; es handelt sich also nicht um einen technischen Unterschied, sondern um einen Stilunterschied.
    Wenn das stimmt, hat der Autor deutlich mehr getan, als nur die Patch-Idee „vorzuschlagen“.
    Er hat das Issue debuggt und den vollständigen Patch geschrieben, der dann mit einer kleinen Änderung akzeptiert wurde.
    1: https://news.ycombinator.com/item?id=37672558
  • Auch diese Vorgehensweise ist möglich, scheint aber besser für Fälle zu passen, die keine Security-Issues sind.
    Bei Security-Issues sind Geschwindigkeit und Korrektheit wichtiger als Formalitäten, und die Kernel-Maintainer haben korrekt gehandelt, indem sie mit Reported-by Anerkennung gegeben haben.
    Suggested-by wäre etwas besser gewesen, aber das ist nicht das, was OP möchte; OP scheint aufgrund eines sehr kleinen Patches das Label „Kernel-Contributor“ gewollt zu haben.

• Wenn ich mir einige Reaktionen in diesem Thread ansehe, hoffe ich, dass manche Leute insbesondere keine Junior- oder Mid-Level-Entwickler managen
  Wenn man einen engagierten Junior im Team hat, der zu einem Issue einen hilfreichen, aber qualitativ noch unzureichenden PR einreicht, ist es die schlechteste Reaktion, ihn mit „nicht gut genug“ abzukanzeln, den Code wegzuwerfen und ihn selbst neu zu schreiben
  Das demotiviert, verschwendet die Zeit dieser Person und die eigene, verhindert, dass sie etwas lernt, sorgt dafür, dass sie es beim nächsten Mal nicht besser macht oder sinnvoll beiträgt, und schreckt sie davon ab, um Hilfe zu bitten oder freiwillig beizutragen
  Selbst wenn sie sich in einen komplexen Codebereich gestürzt hat, gibt es bessere Arten der Interaktion

  • Es ist etwas anderes, in einer Firma oder einem Team Juniors zu managen und zu coachen, als wenn irgendeine Person aus dem Internet einen kleinen Patch schickt
    Bei einem Junior im Team erwartet man, dass er eine Weile bleibt, und durch das Vorstellungsgespräch hat man die grundlegende Kompetenz zumindest teilweise geprüft, sodass man auf sein Entwicklungspotenzial vertrauen kann
    Eine beliebige Person aus dem Internet ist nicht verpflichtet zu bleiben, und solche „Drive-by“-Fixes bleiben meistens nicht
    Man weiß nicht, welche Fähigkeiten diese Person tatsächlich hat oder wie sie auf Änderungswünsche reagieren wird
    Aus Managementsicht ist eines der frustrierendsten Dinge, wenn jemand irgendeine Person in meine Reporting-Line steckt und mich für deren Leistung verantwortlich macht
    Wenn man sich nun vorstellt, dass irgendeine anspruchsvolle Person aus dem Internet ein paar Tage in einen minderwertigen Fix investiert hat und ich sie wie ihr Manager coachen soll, ergibt diese Analogie keinen Sinn
    Früher habe ich mich gefragt, warum es so viel Aufregung wegen unerwünschter PRs gibt, wenn man ein Projekt auf GitHub hostet, aber jetzt verstehe ich es
    Wenn man keine Lust hat, sich mit beliebigen PRs zu beschäftigen, als „schlechter Manager, der demotiviert und Zeit verschwendet“ beschuldigt zu werden, ist ein überzogenes Anspruchsdenken
    Beispiel: https://news.ycombinator.com/item?id=25940799
  • Wenn ein übermäßig motivierter Junior eine Woche lang an etwas gearbeitet hat, das weder Priorität hat noch seine Aufgabe ist, kann man ihn beiseitenehmen und sagen: „Das stellen wir vorerst zurück und schauen es uns später wieder an. Im Moment hat X Priorität, deshalb können wir hier keine Zeit investieren“
    Profi zu werden bedeutet auch zu lernen, die eigene Person vom Arbeitsergebnis zu trennen und zu verstehen, dass Kolleginnen und Kollegen nicht mit einem böswilligen Plan versuchen, einem Steine in den Weg zu legen
    Jeder hat schon erlebt, dass ein Projekt nach Monaten Arbeit eingestellt wurde, oder in einem Code-Review harte Kommentare bekommen
    Solche Dinge anzunehmen und daraus zu lernen gehört zum Dasein als Profi
    Natürlich müssen auch direkte und unhöfliche Menschen lernen, dass diese Haltung viele Beziehungen ruiniert
    Wir sind keine Künstler, sondern Profis
    Wenn ein Junior-Bauingenieur eine Brücke mit strukturellen Mängeln entworfen hat und ein Senior sagt: „Tut mir leid, aber das ist jetzt kaum noch zu retten, wir müssen alles neu machen“, dann macht der Senior nichts falsch
    Ich möchte nicht über eine wacklige Brücke fahren, nur um das Ego des Juniors zu schützen
    Einige Kommentare von Leuten, die offenbar nicht viel Erfahrung mit der Wartung großer Open-Source-Projekte haben, scheinen Maintainer als besonders bösartig zu sehen
    Wenn man ein kleines Open-Source-Projekt wartet, merkt man, dass PRs von beliebigen Leuten viel Arbeit bedeuten und oft mehr Zeit kosten, als es selbst zu machen
    Es kostet viel Zeit, den Code sorgfältig zu reviewen, mit Beitragenden zu kommunizieren, Probleme zu beheben und erwartete Qualität sowie Konventionen einzuhalten
    Ein Junior am Arbeitsplatz hat bereits ein Vorstellungsgespräch durchlaufen, hier dagegen bittet eine beliebige fremde Person um eine Änderung
    Wenn ein PR abgelehnt wird, ist das enttäuschend, aber ich verstehe beide Seiten
    Wenn man sich die Unterhaltung des Maintainers ansieht, wirkt er nicht besonders gemein oder unhöflich
  • Es wäre gut, das noch einmal zu lesen. Es sieht nicht so aus, als sei das tatsächlich so passiert; zumindest könnte es die emotionale Rekonstruktion einer Seite sein
  • Wirklich enttäuschend ist, dass es in diesem Thread viele harte Kommentare gibt
    Es fühlt sich fast an wie ein schlechter Thread auf LKML
    Diese Person hat buchstäblich 95 % der Arbeit gemacht und wird dafür kritisiert, dass sie dafür Anerkennung haben möchte
    Ich verstehe überhaupt nicht, warum
  • Dieser Thread wirkt wie ein Blitzableiter für schlechte zwischenmenschliche Beziehungen, und wenn man an die Altersstruktur der HN-Leserschaft denkt, ist das ziemlich peinlich

• Wenn man die beiden Patches vergleicht, sieht man klare Unterschiede
  Der Beitrag im verlinkten Artikel ändert das Verhalten in beiden Zweigen von if...else, der endgültige Beitrag ändert dagegen nur den Pfad innerhalb des if-Zweigs
  Der Maintainer wird Gründe gehabt haben, den Code zu ändern
  Trotzdem ist der eigentliche Fix nahezu zu 100 % kopiert, daher wäre es angemessen gewesen, dem ursprünglichen Patch-Autor Credit zu geben

  • Dasselbe gilt für den ursprünglichen Patch aus der oben genannten Mail-Unterhaltung
    Das ursprüngliche Ergebnis hat auf der PPC32-Architektur weder flush_fp_to_thread noch eine else-Bedingung
    Der endgültige Patch ruft flush_fp_to_thread(child); auf, behandelt innerhalb der Bereichsprüfung von fpidx abhängig von CONFIG_PPC32 und weist, wenn es außerhalb des Bereichs liegt, fpscr zu
    Daher gibt es echte Unterschiede zwischen den beiden Lösungen
    Ich kann nicht sagen, welche richtig oder besser ist, aber dass die Ergebnisse unterschiedlich sind, ist klar
  • Aus Sicht von jemandem, der lange C verwendet hat, wäre man vorsichtig, wenn Code mit lauter Custom-Makros kommt
    Das könnte auf mangelnde Vertrautheit mit C hindeuten
    Die Wartung großer Codebasen besteht zur Hälfte aus Feuerlöschen und zur Hälfte aus Vermeidung von Namenskollisionen
  • Ich bin der Autor des Artikels. Leider kann ich meinen ursprünglichen Patch nicht verlinken, weil ich ihn nur an die Linux-Kernel-Sicherheits-Mailingliste geschickt habe
    Dieser Patch war ziemlich anders, und ich wünschte, ich hätte ihn irgendwo gespeichert, damit ich ihn zeigen könnte

• Das ist ein ganz normaler Teil von Kernel-Beiträgen.
  Oft ist ein Patch die knappste Art, ein Problem zu vermitteln, also schickt man einen Patch; die Erwartung sollte aber eher sein, dass sie diesen Patch nicht verwenden.
  Spontan fallen mir mindestens drei Fälle ein, in denen mir das passiert ist [1][2][3].
  Ehrlich gesagt wäre ich nie auf die Idee gekommen, mich darüber zu ärgern, denn mein Hauptziel war, den Bug zu beheben, und ich war froh, dass das passiert ist.
  Mein ehrlicher Rat an OP: Diesen Blogpost sollte er besser löschen. Er wird es später bereuen.
  [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/lin...
  [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/lin...
  [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/lin...

  • Jemanden namentlich bloßzustellen und ihn vor den Bus zu werfen, wirkt wirklich nicht gut.
    Ich war ziemlich überrascht, den Namen der anderen Person zu sehen.

• In Open-Source-Projekten passiert so etwas ständig, und am Ende nimmt es einem nur die Lust auf weitere Beiträge.
  Letztes Jahr habe ich ein seit Jahren offenes Issue übernommen, einen PR erstellt, die Tests korrigiert und sogar noch separat Build-Probleme behoben.
  Der Maintainer hat nur den PR für die Build-Probleme übernommen und das ursprüngliche Issue einfach ignoriert.
  Es gibt keinerlei Chance, dass ich noch einmal Zeit in Beiträge zu diesem Projekt stecke.

  • Bei manchen Leuten besteht der ganze GitHub-Lebenslauf aus kopierten und eingefügten Beiträgen.
    Man öffnet einen PR und bekommt Kommentare mit Änderungswünschen.
    Wenn man ein paar Tage später Zeit hat, sie umzusetzen, ist er bereits durch einen anderen PR eines regelmäßigen Contributors geschlossen.
    Schaut man sich diesen PR an, ist er fast identisch mit dem eigenen Code, nur ein Variablenname wurde geändert, und er wird nach „Freundschafts“-Review-Standard sofort genehmigt.
    Sieht man sich das Profil dieser Person an, sind fast alle Beiträge Kopien von Code anderer Leute.
  • Als Maintainer eines kleinen Projekts freue ich mich, wenn Leute Code oder Tests beitragen.
    Ich versuche ihnen zu helfen, alles so aufzuräumen, dass es zum Rest der Codebase passt, aber genau das ist am schwierigsten.
    Die meisten Entwickler wollen ihre Änderung einfach durchdrücken, statt zuerst die Konventionen des Projekts zu lernen.
    Wenn der Code aber ungefähr passt und ich nach dem Merge noch etwa 15 Minuten aufräumen und ein paar Tests ergänzen muss, nehme ich ihn trotzdem an.
    Dieser Fall sieht ebenfalls nach so einer Situation aus, und der Maintainer hätte genau das tun sollen.
    Der Contributor hätte die Anerkennung bekommen, und der Maintainer hätte nur ein paar Minuten für das nötige Aufräumen investieren müssen, damit es upstream akzeptiert wird; alle wären zufrieden gewesen.
    „Meine Version ist besser“ zu sagen, ist sehr arrogant und unprofessionell, egal ob sie tatsächlich besser ist oder nicht.
    Wenn ein Issue sechs Jahre nach der ersten Meldung nicht behoben wurde, sollte man anerkennen, dass es ohne den Beitrag dieser Person auch den eigenen Fix nicht gegeben hätte.
    Nachdem man die „Lösung“ gesehen hat, den Fix zu übernehmen, kommt praktisch einem Diebstahl nahe.
  • Meiner Erfahrung nach wollen Code-Projekte vor allem Bug-Reports und Verbesserungsideen, nicht unbedingt deinen Code.
    Leute nehmen es persönlich, wenn ihr Code nicht angenommen oder neu geschrieben wird, übersehen dabei aber, dass das Schreiben von Code nur ein kleiner Teil des Betriebs eines Projekts ist.
  • Das ist ein häufiges Problem.
    Ich schaue mir normalerweise Bug-Reports und Merge Requests an, um abzuschätzen, wie ein Patch behandelt werden würde.
    Bei Projekten, die nicht nach guten Zielen für Beiträge aussehen, habe ich viele Bugfixes einfach aufgegeben; der Linux-Kernel von 2003 war auch so ein Beispiel.
    Manche Projekte sind einfach so, da hält man sich besser heraus, und meistens ist es Zeitverschwendung.
    Inzwischen werden nach der KI-Revolution Open-Source-Lizenzen breitflächig verletzt; seitdem habe ich keinen Open-Source-Code mehr eingereicht und werde das auch in Zukunft nicht tun.

• Ich finde die Toxizität hier wirklich schwer nachvollziehbar.
  Aussagen wie „Man sollte es nicht für Anerkennung, Sterne, Belohnung oder den Lebenslauf tun“ klingen völlig verrückt.
  Haben die Leute, die so etwas sagen, Jobs? Falls ja, müssten sie konsequenterweise komplett kostenlos arbeiten.
  Auch Gehalt ist eine Form der Anerkennung für Beiträge.
  Selbst wenn die endgültige Lösung anders ist als der ursprüngliche Vorschlag: Wenn sie auf der Entdeckung der ursprünglichen Lösung beruht, ist es eine sehr kleine Sache, einen Co-Autor hinzuzufügen.
  Der Autor hat einen berechtigten Grund, sich darüber zu beschweren, und es wird weder das erste noch das letzte Mal sein, dass jemand das Gefühl hat, sein Beitrag werde nicht anerkannt.

  • Die Leute tun so, als wären wir Code-Roboter, die keinerlei Anerkennung für gute Arbeit wollen.
    Selbst wenn der Patch selbst minderwertig war, war es die Debugging-Arbeit ganz sicher nicht.
  • Das ist eine andere Situation als ein Arbeitsverhältnis.
    Bei einer Anstellung einigen sich Arbeitgeber und Arbeitnehmer darauf, dass Arbeit gegen Vergütung geleistet wird.
    Wenn man unangekündigt in irgendeine Firma hineinspaziert und anfängt, Arbeit zu erledigen, um die niemand gebeten hat, würden nur wenige behaupten, man habe Anspruch auf Vergütung.
    Genau das ist die passendere Analogie zu dem, was OP passiert ist.
  • Es wäre fairer gewesen, den ursprünglichen Autor anzuerkennen und sich selbst als Editor darzustellen.

• Eine Reaktion nach dem Muster „Tut mir leid, aber meine Version ist besser. Wenn du Linux-Kernel-Contributor werden willst, hier ist ein Issue, das du beheben kannst“ ist ein hervorragender Weg, jemanden dazu zu bringen, nie wieder zu helfen, und auch anderen die Motivation zu nehmen

  • Die tatsächliche Formulierung war anders
    Der Maintainer sagte, er habe den Crash mit gdbserver nicht reproduzieren können, es gebe aber einen Testcase, der den Bug zeige, daher habe er ihn überprüft und den Fix getestet; außerdem schrieb er: „Danke für den Patch, aber ich wollte es anders beheben. Könntest du den untenstehenden Patch ausprobieren und bestätigen, ob der Bug behoben ist?“
    https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-June/24...
    Es gab außerdem weitere Review-Kommentare, die nicht erwähnt wurden
    Darin hieß es, es sei nicht klar, welchen Vorteil ein nur für PPC32 gedachtes Makro habe, #ifdef solle man möglichst vermeiden, und Michaels Patch sei leichter zu verstehen
    https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-June/24...
  • Der Maintainer hat das tatsächlich nicht so gesagt
    Der OP legt dem Gegenüber Worte in den Mund und ärgert sich dann darüber; besonders in textbasierter Kommunikation ist das keine gute Strategie
  • Gerade wenn der OP zuvor geschrieben hat, dass dieses Issue vor sechs Jahren gemeldet und dann ignoriert wurde, ist das Verhalten des Maintainers ziemlich daneben
  • In Wirklichkeit ist es überhaupt nicht so passiert
    Der OP lässt die Unterhaltung unhöflich wirken, aber tatsächlich war sie das nicht
  • Außerdem: Welche Garantie gibt es, dass der Maintainer beim nächsten Mal nicht wieder „meine Version ist besser“ sagt und auf einen weiteren Bug verweist?