私の最初のカーネル貢献が奪われた
(ariel-miculas.github.io)- Ariel Miculas は PowerPC32 における gdbserver のマルチスレッドデバッグ が壊れる問題を追跡し、Linux カーネルのメモリ破損バグを発見したが、最終的な修正はメンテナーによる別パッチとして取り込まれ、
Reported-byのクレジットしか得られなかった - 手がかりは、同じ症状を扱った過去のメールスレッドと、
task_struct内のthread_struct threadの配置変更 コミットだった。これにより一部のスレッドが誤ったプロセス状態に留まり、gdbserver が制御を失う現象につながっていた - 原因分析には
pahole、ftrace、PowerPC の DABR ベースのハードウェアブレークポイントが使われ、ptrace_put_fprのバッファオーバーフローがtask_struct.__stateのようなフィールドを上書きしていることが確認された - オーバーフローは 32 ビット要素向けのインデックスを 64 ビット配列に適用したことで発生し、32 個しかない
fp_state.fpr配列の後ろへ、ユーザー側の gdbserver が最大 256 バイトまで書き込める状態になっていた - PowerPC メンテナーの Michael Ellerman は独自の修正案を適用し、その後の返信では、
Reported-byだけでは貢献が適切に反映されておらず、パッチを一緒に発展させるべきだったという 謝罪 が述べられた
PowerPC32 で壊れていた gdbserver デバッグ
- 約 1 年半前、Ariel Miculas は前職で、プロジェクトのデバッグ機能を妨げていた問題を調査する時間を求めた
- 問題は PowerPC32 アーキテクチャで gdbserver がマルチスレッドアプリケーションを正しくデバッグできないことだった
- gdbserver の接続が切れ、デバッグセッションをそれ以上制御できなくなっていた
- 原因が toolchain、gdbserver、Linux kernel、カスタムカーネルパッチのどこにあるのかはっきりしていなかった
- 既存の調査と検索の末、同じ症状を扱ったメールスレッドを見つけた
- このスレッドは問題を引き起こしたコミットも指していた
- そのパッチは
task_structの途中にあったthread_struct thread定義を末尾へ移す変更だった
スレッド状態からメモリ破損へと絞り込まれた手がかり
- Holger Brunck の既存分析によると、gdbserver は各スレッドごとにカーネルへ
SIGSTOPを送り応答を待っていたが、エラー時にはカーネルが一部のシグナルにしか応答しなかった - 低レベルでは、gdbserver と相互作用した後に一部のスレッドが誤った プロセス状態 に置かれ、gdbserver はその後制御権を失っていた
- Ariel は PowerPC 関連コミットの説明と
task_struct周辺の変更を 3〜4 日かけて読み込み、後続のカーネルバージョンで問題が解決されているか確認したが、解決策は見つからなかった thread_struct threadの位置を入れ替えながら再現条件を確認し、paholeでtask_structのレイアウトを調べたftraceでデバッグ対象プロセスのスレッドがいつスケジュールされるかを追跡した- 停止したスレッドは、他のスレッドと違って 1 回しかスケジュールされなかった
- この観察によって メモリ破損 の可能性を再び疑うようになった
ハードウェアブレークポイントで突き止めた実際の原因
- x86 の debug registers のように、PowerPC にも DABR register を使った同様の機能があることを確認した
- Linux でハードウェアブレークポイントを使う方法を調べた後、Stack Overflow の回答をもとにカーネルモジュールを実装した
- このモジュールで
task_struct.__stateフィールドにハードウェアブレークポイントを設定し、誰が値を書いているのかを追跡した - カスタムカーネルモジュールは
task_struct.__stateに書き込む箇所のスタックトレースを示し、その中の不審な経路がptrace_put_fprの バッファオーバーフロー へとつながっていた ptrace_put_fprは POKEUSER API で使われており、このオーバーフローは__stateのようなtask_structの重要なフィールドを上書きしていた__stateはプロセス状態を保持する- カーネルはデバッガによって停止されたプロセスを追跡する際にもこのフィールドを使う
最大 256 バイトまで越境し得た配列書き込み
- オーバーフローの原因は、32 ビット要素配列に対して書き込むよう設計されたインデックスを 64 ビット要素配列 に適用していたことにあった
- FPR を指すインデックスは 64 個あり、64 ビット要素基準でのアクセス範囲は
64 * 8 = 512バイトだった - 実際の
fp_state.fpr配列には 32 エントリしかなく、利用可能なメモリは32 * 8 = 256バイトだった - その結果、ユーザー、つまり gdbserver が配列末尾を越えて最大 256 バイトまで書き込めていた
アップストリーム提出とクレジットを巡る対立
- Ariel は、プロセス状態メモリを上書きできるこのメモリ破損問題にはセキュリティ上の影響があり得ると考え、
security@kernel.orgにパッチを送った- このメーリングリストは非公開のため、元の提出パッチにはリンクできなかった
- PowerPC メンテナーの Michael Ellerman は個人的に連絡し、この問題に対応すると返答した
- Ariel は 2 つの修正パッチを送った
- セキュリティメーリングリストへ送った元のパッチ
- 元の提出に対する提案を反映した別バージョンのパッチ
- 2 つ目のパッチは、PowerPC64 上で PowerPC32 の動作をエミュレートしていた既存カーネルコードをベースにしており、そのコードは FPR インデックス処理を正しく行っていた
- Michael Ellerman はどちらのパッチも受け入れず、独自の修正案を実装した
- Ariel は、この問題を修正した功績と最初のカーネル貢献者としてのクレジットのために、自分のパッチが受け入れられることを望んでおり、フィードバックを反映した後続バージョンを送る意思もあった
- 実際に得られたクレジットは
Reported-byタグだけで、Ariel は、原因分析・修正・テスト・検証・社内フィードバックの反映・最新カーネルへの適用に費やした努力に比べて不当だと感じた- Linux 文書上、
Reported-byはバグを見つけて報告した人にクレジットを与えるタグである - この問題自体はすでに 6 年前に報告されていた
- Linux 文書上、
更新と謝罪の返信
- Hacker News のユーザーは、Michael Ellerman が「パッチをありがとう。ただ、別の形で直したい。以下のパッチを試して、バグが修正されるか確認してもらえるか」と書いたメールを指摘した
- このメールは、Michael Ellerman が Ariel のパッチをレビューする代わりに独自実装を選び、Ariel にそのパッチのテストを依頼した状況を示している
- Ariel はセキュリティメーリングリストに送っていた元のパッチを見つけ、linuxppc-dev にメールスレッドを転送した
- その後の更新には、パッチ処理の進め方に対する謝罪の返信が含まれていた
- 「あなたのパッチを一緒に発展させるために、もっと時間を使うべきだった」という内容が含まれていた
Reported-byタグが Ariel の貢献を適切に反映していなかったという内容も述べられていた
1件のコメント
Hacker News の意見
パッチ全体を受け入れないにしても、Ariel Miculas に功績を認めるほうが適切だった
このセキュリティ問題は、彼が見つけて修正案を送らなければ、誰も直さなかった可能性が高い
Michael が彼のパッチを読み、スタイルだけ一部変えて自分の名前で提出したのだとしたら、功績を認めないのは非倫理的だし、たとえコードがひどかったとしても、それはなお共同作業だったのだから、そんな扱いを受けたら二度と一緒に仕事をしたいとは思わないだろう
後者はそれでも
Co-Authored-Byが残るが、誰かが自分のプロジェクトのコードをコミット履歴なしで自分のリポジトリに投げ込み、「フォーク」しているのを見て初めて、貢献者の立場ではかなり苦い経験なのだと気づいたコードが悪かったとしても、マージされたリファクタリング済みの修正は、そのコードなしには存在しなかったはずで、「自分の版のほうが良い」という言い方はかなり軽視しているように受け取られうる
ただし、メンテナの実際の回答をリンクした返信を見ると、最後の文は成り立たない
しかしこの場合、投稿者はそのパッチの共同著者に近い
学術出版にたとえるなら、誰かにプレプリントを送ったところ、その人が同じテーマで論文を出し、謝辞にだけ載せて共同著者には入れなかったようなものだ
ここでは出版ではなくカーネル開発だが、著者表示は職務経歴書では今でも重要だ
スタイルや構造の一貫性についてのフィードバックは必要だが、優れたメンテナとは功績を認め、裏方として物事が円滑に進むようにする人だ
そのとき Ted Tso が私の作業の正確さに満足してパッチを入れたが、私は功績を求めもしなかったし、受け取りもしなかった
関心があったのはカーネルの改善だけで、名前を残すことには興味がなかった
数年後に別件で貢献者リストに名前が入ることにはなったが、どちらにしても気にしていなかった
Reported-by: Ariel Miculasとあり、正確に見えるメンテナのパッチのほうが良い
fpidx < (PT_FPSCR - PT_FPR0)のチェックを失っていないput関数でfpidxが範囲外のときに使われる*data = child->thread.fp_state.fpscr;という代替代入を失っていないTS_FPRWIDTHが 1 なら恒等式に簡約される不要なFPRINDEXマクロを避けている元のパッチのコミットメッセージは「PPC32 では
TS_FPRWIDTHが 1 だと仮定してよい」としていながら、FPRNUMBER、FPRHALF、FPRINDEXを含めていた単に修正可能な配列の不一致を見つけたからといって、バグのあるパッチをそのまま受け入れるべきだということにはならない
ただしメンテナは、Miculas が単なる報告をしただけでなく、調査して根本原因を見つけ、ほぼ同じ簡単な修正の自分版まで作ったことは認めるべきだった
Reported-byは、当事者が徹底的に調査し、原因まで突き止めたというニュアンスを含めるには不正確だ新しい貢献者に対してはプロジェクトでよくそうするし、それが正しいやり方だ
人を教え、順応させる過程なのに、作業をただやり直してしまうと、貢献意欲をくじき、学ぶ機会も奪ってしまう
2番目の版を書くのはいつでもより簡単だ
これを単純な修正だと言うのは、ソフトウェア開発においてバグ修正にかかる作業量を理解していないということだ
ここで Michael も、最初は再現できず、後になってようやく確認できたと認めている
これは OP がデバッグと修正に費やした作業量をよく示しており、その作業を奪うのは正しくない
原文では、パッチを受け入れられて功績を得てカーネル貢献者になりたかったし、フィードバックを反映して後続版も送るつもりがあった、と述べていた
メンテナは OP のコードをチェックアウトし、望む変更を加えたうえで共同著者としてコミットするために必要な2分ほどを使えばよかった
そうすれば最良の修正がマージされ、他人の作業も認められる
特に OP は功績を認められたいと明確に表明していたのだから、それが最善だった
ただ、こうしたリポジトリのメンテナたちは社会的な礼儀をしばしば見落とすようなので、驚きはしない
PR はさまざまな理由で受け入れられないことが多いが、気にしていない
コードを公開しておくために PR を送っているのであって、マージされるほど「良く」するために時間を使うつもりはない
そのまま提供し、誰かが十分気に入れば、その人が手続きを処理すればよい
そのままマージされたものもあれば、リポジトリ所有者が少し直してマージしたものもある
オープンソースのコードは栄光や人気のために書いているのではなく、好きだからやっている
80年代と90年代が懐かしい
Linux カーネルは報酬ではなく、投稿者には共感するものの、コミュニティから来た版よりも保守しやすい解決策を好むのは正しい
そのため、メンテナーは権限の範囲内にいた
とはいえ、修正に貢献した人にクレジットを表示するのはとても簡単
Node.js ではリリースノートでクレジットを与え、PR の一部の作業を取り込んで修正したときは
Co-Authored-By:を追加するよう努めている長くメンテナンスしていると、自分にとってクレジットの重要性は薄れていくため、新しいコントリビューターにとってそれが非常に重要だという事実を忘れがち
このケースのように、クレジットの共有が投稿者にとって重要であり、また当然でもあった状況で、より配慮できなかったのは投稿者と該当する Linux 領域の双方にとって損失である
コミュニティの努力に依存するプロジェクトが持続するやり方ではない
Linux カーネルのように大きく尊敬されているプロジェクトなら、プロジェクトにとって最善のことも考え、新しいコントリビューターも尊重できる人たちがいると思っていた
新しいコントリビューターだからという理由だけで、最善ではない解決策を受け入れようという人はいないだろう
正しいやり方は、パッチをレビューし、何がなぜ問題なのかを説明し、どう書き直すとよいかを提案し、必要ならその部分を一緒に書いて共著者になること
そうしてこそ、今後も関心を持つコントリビューターの流れが生まれる
Reported-Byタグでクレジットを受けたように見えるカーネル文書では、タグの慣例の一つとして
Suggested-byが定義されているSuggested-by:タグは、パッチのアイデアをその人が提案したことを示し、そのアイデアへのクレジットを保証する特にそのアイデアが公開フォーラムに投稿されたものでない場合、提案者の許可なく追加してはならないとされている
アイデアの提供者に誠実にクレジットを与えれば、彼らが今後もまた助けてくれる可能性が高まる、という趣旨も書かれている
この状況では
Suggested-byのほうがより適切だったかもしれない問題の解決策は見つけたが、プロジェクトに慣れていないためパッチの正確な文法は維持されなかった、という意味をよりよく伝える
参考: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/lin...
一方は
ifの外側のifdefにあり、もう一方は内側のif文にあるという違いだけで、技術的な差ではなくスタイルの差だということそうだとすれば、著者は単にパッチのアイデアを「提案」した以上のことをしている
issue をデバッグし、パッチ全体を書き、受け入れられたのは小さな変更一つを加えたものだったということ
1: https://news.ycombinator.com/item?id=37672558
セキュリティ issue では形式よりも速度と正確性のほうが重要で、カーネルメンテナーたちは
Reported-byでクレジットを与えた点で正しく対応したSuggested-byのほうが少しよいだろうが、投稿者が望んでいるものではなく、投稿者はごく小さなパッチを根拠に「カーネルコントリビューター」という印を望んでいるように見えるこのスレッドの反応の一部を見ると、何人かは特にジュニアや中堅開発者を管理しないでほしいと思う
チームに熱意のあるジュニアがいて、ある issue には役立つが品質は不足している PR を出したなら、「十分によくない」と叱ってコードを捨て、自分で書き直すのは最悪の対応
士気をくじき、その人と自分の時間を無駄にし、何も学べないようにし、次回は改善したり意味のある貢献をしたりできなくし、助けを求めることや自発的な貢献もためらわせる
複雑なコード領域に飛び込んだ場合でも、もっとよい関わり方はある
チームのジュニアはしばらく在籍するという期待があり、面接を通じて基本的な能力もある程度確認しているので、成長の可能性を信頼できる
インターネット上の任意の人には残り続ける義務がなく、こうした「ドライブバイ」の修正はたいてい残らない
その人に実際どんな能力があるのか、修正依頼にどう反応するのかも分からない
マネジメントの観点で最もいら立つことの一つは、誰かが任意の人を自分の報告ラインに入れ、その人の成果に責任を持たせること
いまや、インターネット上の権利意識の強い誰かが数日かけて低品質な修正を作り、私がその人のマネージャーのようにコーチングすべきだと想像すると、この比喩は成り立たない
GitHub でプロジェクトをホストすると、望まない PR のせいでなぜ大騒ぎになるのか以前は不思議だったが、今は理解できる
任意の PR の相手をしたくないと、「士気をくじき時間を無駄にさせる悪いマネージャー」だと非難されるというのは、過剰な権利意識だ
例: https://news.ycombinator.com/item?id=25940799
プロになるということは、自分自身と成果物を切り離し、同僚が悪意ある計画で自分の邪魔をしようとしているわけではないと学ぶことでもある
誰でも、数か月取り組んだプロジェクトが中止になったことがあり、コードレビューで厳しいコメントを受けたことがある
そうしたことを受け止めて学ぶのは、プロとしての務め
もちろん、率直で無礼な人たちも、その態度が多くの関係を壊すことを学ぶべき
私たちは芸術家ではなくプロだ
ジュニアの土木エンジニアが構造上の欠陥がある橋を設計し、シニアが「申し訳ないが、これはもう救いようがないので全部やり直す必要がある」と言ったなら、シニアに問題はない
ジュニアの自尊心を守るために、欠陥のある橋を車で渡りたいとは思わない
大規模なオープンソースプロジェクトをメンテナンスした経験があまりないコメントが、メンテナーをやたら冷酷だと見ているように思う
小さなオープンソースプロジェクトを維持してみると、任意の人からの PR を受けるのは手間が多く、しばしば自分でやるより時間がかかる
コードを綿密にレビューし、コントリビューターとやり取りして問題を直し、期待する品質や慣例に合わせるのに多くの時間がかかる
職場のジュニアはすでに面接を通過しているが、ここでは任意の見知らぬ人が変更を求めている
PR が拒否されればつらいが、双方の立場はどちらも理解できる
メンテナーのやり取りを見る限り、特に意地悪だったり無礼だったりするようには見えない
ほとんど LKML の悪いスレッドのように感じる
その人は文字どおり 95% の作業をしたのに、その功績を認めてほしいと言っていることで批判されている
いったいなぜなのか分からない
2 つのパッチを比べると明確な違いが見える
リンク先の記事の貢献は
if...elseの両方の分岐の動作を変えているが、最終的な貢献はif分岐内の経路だけを変えているメンテナーがコードを変えたのには理由があったのだろう
それでも実際の修正はほぼ 100% コピーなので、元のパッチ作者に功績を与えるのが適切だった
元の結果では PPC32 アーキテクチャで
flush_fp_to_threadもなく、else条件もない最終パッチは
flush_fp_to_thread(child);を呼び出し、fpidxの範囲チェック内でCONFIG_PPC32かどうかに応じて処理した後、範囲外ならfpscrを代入するだから 2 つの解法の間には実際の違いがある
どちらが正しい、またはよりよいかは言えないが、結果が異なることは明らか
C に対する未熟さを示している可能性もある
大きなコードベースの保守は、半分は火消しで、半分は名前の衝突回避だ
そのパッチはかなり違っていて、見せられるようにどこかへ保存しておけばよかったです
これはカーネルへの貢献では一般的なことの一部です。
問題を最も簡潔に伝える方法がパッチである場合が多いのでパッチを送るのであり、期待値としては、彼らはそのパッチを使わないだろう、という側に置くべきです。
思い付くだけでも、私にはこういうことが少なくとも3回 [1][2][3] ありました。
正直、怒ろうと考えたことすらありません。私の主な目的はバグを直すことで、それが実現してうれしかったからです。
OPに心から勧めるなら、このブログ記事は削除したほうがよいです。後で後悔するでしょう。
[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...
他人の名前を見てかなり驚きました。
オープンソースプロジェクトではこういうことは常に起きていて、結局は追加で貢献しようという意欲を削ぐだけです。
昨年、数年間開いたままだった issue を引き受け、PR を作り、テストコードも直し、ビルド問題を直す別作業までしました。
メンテナーはビルド問題を直す PR だけを取り込み、元の issue はそのまま無視しました。
そのプロジェクトに再び時間を使って貢献する可能性はまったくありません。
PR を開き、変更依頼のコメントを受けます。
数日後、時間ができて直そうとすると、常連コントリビューターが出した別の PR ですでにクローズされています。
その PR を見ると私のコードとほぼ同じで、変数名が1つ変わっているだけで、「お友達」レビュー基準で即座に承認されています。
その人のプロフィールを見ると、ほとんどすべての貢献が他人のコードをコピーしたものです。
コードベースの他の部分に合うように整理する手助けをしようとしていますが、これが最も難しいところです。
たいていの開発者は、まずプロジェクトの慣習を学ぶより、自分が直した内容をそのまま押し込みたがるからです。
それでもコードがおおむね合っていて、マージ後に私が15分ほど整えてテストを少し足せばよい程度なら受け入れます。
今回もそういう状況に見えますし、メンテナーはそうすべきでした。
貢献者は功績を得て、メンテナーはアップストリームに受け入れられる程度に必要な整理を数分するだけで、全員が幸せになれたはずです。
「私のバージョンのほうが優れている」という発言は、実際に優れているかどうかにかかわらず、非常に尊大でプロらしくありません。
issue が最初に報告されてから6年間直せなかったのなら、その人の貢献がなければあなたの修正もなかった、という点を認めるべきです。
「解法」を見た後でその修正を持っていくのは、実質的に盗みに近いです。
人は自分のコードが受け入れられなかったり書き直されたりすると個人的に受け止めますが、プロジェクトを運営するうえでコードを書くことは小さな部分にすぎない、という点を見落としています。
私はたいてい、バグレポートやマージリクエストを見て、パッチがどう扱われるかを見極めます。
貢献しやすそうに見えないプロジェクトでは、多くのバグ修正をそのまま諦めましたし、2003年の Linux カーネルもその一例でした。
プロジェクトによっては単にそういうものなので、関わらないほうがよく、たいてい時間の無駄になります。
今では AI 革命以降、オープンソースライセンスが広範に違反されているので、それ以来オープンソースコードを提出していませんし、今後もしません。
ここでの毒々しさは本当に理解しがたいです。
「認知、スター、報酬、履歴書のためにやるべきではない」といった言い方は、正気とは思えません。
そう言う人たちは仕事を持っているのでしょうか？ あるなら、筋を通すには完全に無料で働くべきです。
給与も貢献に対する認知の一形態です。
個人的には、最終的な解法が提案されたものと違っていたとしても、元の解法の発見に基づいているなら、共同著者を追加するのはごく小さな手間です。
著者にはこの点について不満を述べる正当な理由があり、誰かの貢献が認められていないと感じるのは、これが最初でも最後でもないでしょう。
パッチ自体が劣っていたとしても、デバッグの努力は決してそうではありませんでした。
雇用では、雇用主と従業員が報酬と引き換えに仕事をするという合意を結びます。
予告なしにどこかの会社に入り、誰にも頼まれていない仕事を始めた場合、報酬を受け取る権利があると主張する人はほとんどいないでしょう。
OPに起きたことにより正確なたとえは、こちらです。
「悪いけど、私の版のほうがいい。Linux カーネルのコントリビューターになりたいなら、ここに直せる Issue があるよ」というような対応は、誰かが二度と手伝わないようにさせ、ほかの人の意欲までくじく見事な方法だ
メンテナーは
gdbserverではクラッシュを再現できなかったが、バグを示すテストケースがあるので確認して修正をテストしたと言い、「パッチありがとう。ただ、別の直し方をしたかった。以下のパッチを試して、バグが直るか確認してもらえるか？」と書いていたhttps://lists.ozlabs.org/pipermail/linuxppc-dev/2022-June/24...
さらに、言及されていない別のレビューコメントもあった
PPC32 だけのためのマクロにどんな利点があるのか分からず、
#ifdefは可能な限り避けるべきで、Michael のパッチのほうが理解しやすいという内容だったhttps://lists.ozlabs.org/pipermail/linuxppc-dev/2022-June/24...
OP が相手の口に言葉を押し込んで、それに腹を立てているのであって、特にテキストでのやり取りでは良い戦略ではない
OP は会話が無礼だったかのように見せているが、実際にはそうではなかった