| Message ID | 20250308155011.1742461-1-kent.overstreet@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | dm-flakey: Fix memory corruption | expand |
On Sat, Mar 08, 2025 at 10:50:08AM -0500, Kent Overstreet wrote: > So, this code clearly isn't getting tested - at all. Besides this bug, > the parsing for the "corrupt" modes is also broken. > > Guys, don't push broken crap, and figure out how to write some tests. Thank you sir, may we have another? Like you never introduced a bug in your life? Not going to tolerate your entitled primadonna attitude here. You are capable of being better, you've chosen not to be on this issue (twice) > -- >8 -- Also, your patch header is pathetically bad. But we get it, you're far too busy to write a proper patch header that explains your change. Or adds a Fixes tag, e.g.: Fixes: 1d9a94389853 ("dm flakey: clone pages on write bio before corrupting them") > Cc: dm-devel@lists.linux.dev > Cc: Mikulas Patocka <mpatocka@redhat.com> > Cc: Mike Snitzer <snitzer@kernel.org> > Cc: Alasdair Kergon <agk@redhat.com> > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > --- > drivers/md/dm-flakey.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) And what's with your inaccurate diffstat? > diff --git a/drivers/md/dm-flakey.c b/drivers/md/dm-flakey.c > index 731467d4ed10..5ceb5d68f65c 100644 > --- a/drivers/md/dm-flakey.c > +++ b/drivers/md/dm-flakey.c > @@ -426,7 +430,7 @@ static struct bio *clone_bio(struct dm_target *ti, struct flakey_c *fc, struct b > if (!clone) > return NULL; > > - bio_init(clone, fc->dev->bdev, bio->bi_inline_vecs, nr_iovecs, bio->bi_opf); > + bio_init(clone, fc->dev->bdev, clone->bi_inline_vecs, nr_iovecs, bio->bi_opf); > > clone->bi_iter.bi_sector = flakey_map_sector(ti, bio->bi_iter.bi_sector); > clone->bi_private = bio; > -- > 2.47.2 >
On Sat, Mar 08, 2025 at 01:19:30PM -0500, Mike Snitzer wrote: > On Sat, Mar 08, 2025 at 10:50:08AM -0500, Kent Overstreet wrote: > > So, this code clearly isn't getting tested - at all. Besides this bug, > > the parsing for the "corrupt" modes is also broken. > > > > Guys, don't push broken crap, and figure out how to write some tests. > > Thank you sir, may we have another? > > Like you never introduced a bug in your life? > > Not going to tolerate your entitled primadonna attitude here. You are > capable of being better, you've chosen not to be on this issue (twice) Talking about basic engineering standards is in no way "being a prima donna". Testing your changes is as basic as it gets, and this code wasn't tested _at all_.
On Sat, Mar 08, 2025 at 04:50:05PM -0500, Kent Overstreet wrote: > On Sat, Mar 08, 2025 at 01:19:30PM -0500, Mike Snitzer wrote: > > On Sat, Mar 08, 2025 at 10:50:08AM -0500, Kent Overstreet wrote: > > > So, this code clearly isn't getting tested - at all. Besides this bug, > > > the parsing for the "corrupt" modes is also broken. > > > > > > Guys, don't push broken crap, and figure out how to write some tests. > > > > Thank you sir, may we have another? > > > > Like you never introduced a bug in your life? > > > > Not going to tolerate your entitled primadonna attitude here. You are > > capable of being better, you've chosen not to be on this issue (twice) > > Talking about basic engineering standards is in no way "being a prima > donna". Testing your changes is as basic as it gets, and this code > wasn't tested _at all_. "entitled primadonna attitude" was me pulling punches. I don't disagree that this is a bug that was missed and that proper testing hasn't been performed (I'd quibble with the no testing part only because I cannot speak for Mikulas and don't like to assume I know it all). But you're missing the very problematic detail: you used a bug in an optional feature of the test-only dm-flakey target to try to take a pound of flesh while preaching from your high horse. That is unacceptable behaviour that won't be tolerated here. Be cool and others will be in return (unless you keep setting fire to bridges). Fin.
On Sun, Mar 09, 2025 at 11:44:38AM -0400, Mike Snitzer wrote: > On Sat, Mar 08, 2025 at 04:50:05PM -0500, Kent Overstreet wrote: > > On Sat, Mar 08, 2025 at 01:19:30PM -0500, Mike Snitzer wrote: > > > On Sat, Mar 08, 2025 at 10:50:08AM -0500, Kent Overstreet wrote: > > > > So, this code clearly isn't getting tested - at all. Besides this bug, > > > > the parsing for the "corrupt" modes is also broken. > > > > > > > > Guys, don't push broken crap, and figure out how to write some tests. > > > > > > Thank you sir, may we have another? > > > > > > Like you never introduced a bug in your life? > > > > > > Not going to tolerate your entitled primadonna attitude here. You are > > > capable of being better, you've chosen not to be on this issue (twice) > > > > Talking about basic engineering standards is in no way "being a prima > > donna". Testing your changes is as basic as it gets, and this code > > wasn't tested _at all_. > > "entitled primadonna attitude" was me pulling punches. > > I don't disagree that this is a bug that was missed and that proper > testing hasn't been performed (I'd quibble with the no testing part > only because I cannot speak for Mikulas and don't like to assume I > know it all). > > But you're missing the very problematic detail: you used a bug in an > optional feature of the test-only dm-flakey target to try to take a > pound of flesh while preaching from your high horse. That is > unacceptable behaviour that won't be tolerated here. Be cool and > others will be in return (unless you keep setting fire to bridges). > > Fin. Mike, saying code needs to be tested is not an "entitled primadonna attitude". Pushing completely broken code because you made no attempt to test it and then flipping out when called out over it - that is. To recap, we're not talking about some obscure corner cases, we're talking about core documentated functionality in dm-flakey that is completely broken in ways that show up immediately if you run it - and there's at least three bugs that I saw; the parsing code, the clone_bio() memory corruption, and the read side corruption still wasn't working when I fixed or worked around the other two (write side did). This isn't your personal project, this is the kernel - there are standards, and other people depend on your work. dm-flakey is used heavily by filesystem folks, and additionally, md-faulty was recently removed because, supposedly, dm-flakey was sufficient. And that's what I was using before, and it worked fine, so I'm willing to bring it back and maintain it if dm-flakey can't be relied on.
On Sun, Mar 09, 2025 at 01:04:22PM -0400, Kent Overstreet wrote: > On Sun, Mar 09, 2025 at 11:44:38AM -0400, Mike Snitzer wrote: > > On Sat, Mar 08, 2025 at 04:50:05PM -0500, Kent Overstreet wrote: > > > On Sat, Mar 08, 2025 at 01:19:30PM -0500, Mike Snitzer wrote: > > > > On Sat, Mar 08, 2025 at 10:50:08AM -0500, Kent Overstreet wrote: > > > > > So, this code clearly isn't getting tested - at all. Besides this bug, > > > > > the parsing for the "corrupt" modes is also broken. > > > > > > > > > > Guys, don't push broken crap, and figure out how to write some tests. > > > > > > > > Thank you sir, may we have another? > > > > > > > > Like you never introduced a bug in your life? > > > > > > > > Not going to tolerate your entitled primadonna attitude here. You are > > > > capable of being better, you've chosen not to be on this issue (twice) > > > > > > Talking about basic engineering standards is in no way "being a prima > > > donna". Testing your changes is as basic as it gets, and this code > > > wasn't tested _at all_. > > > > "entitled primadonna attitude" was me pulling punches. > > > > I don't disagree that this is a bug that was missed and that proper > > testing hasn't been performed (I'd quibble with the no testing part > > only because I cannot speak for Mikulas and don't like to assume I > > know it all). > > > > But you're missing the very problematic detail: you used a bug in an > > optional feature of the test-only dm-flakey target to try to take a > > pound of flesh while preaching from your high horse. That is > > unacceptable behaviour that won't be tolerated here. Be cool and > > others will be in return (unless you keep setting fire to bridges). > > > > Fin. > > Mike, saying code needs to be tested is not an "entitled primadonna > attitude". Definition of primadonna: "a very temperamental person with an inflated view of their own talent or importance." My issue from the start on Friday night (in private) has always been how holier-than-thou yet abusive you've been since having discovered this bug in dm-flakey's optional "corrupt_bio_byte" feature. > Pushing completely broken code because you made no attempt to > test it and then flipping out when called out over it - that is. I didn't push commit 1d9a94389853 _because_ I "made no attempt to test it". Commit 1d9a94389853 sought to fix a similar but different corruption in the original "corrupt_bio_byte" implementation (which proved useful for the specific case it was first developed for). > To recap, we're not talking about some obscure corner cases, we're > talking about core documentated functionality in dm-flakey that is > completely broken in ways that show up immediately if you run it - and > there's at least three bugs that I saw; the parsing code, the > clone_bio() memory corruption, and the read side corruption still wasn't > working when I fixed or worked around the other two (write side did). > > This isn't your personal project, this is the kernel - there are > standards, and other people depend on your work. dm-flakey is used > heavily by filesystem folks, and additionally, md-faulty was recently > removed because, supposedly, dm-flakey was sufficient. > > And that's what I was using before, and it worked fine, so I'm willing > to bring it back and maintain it if dm-flakey can't be relied on. This dm-flakey corrupt_bio_byte bug will be fixed upstream this week. But your recap is devoid of any understanding that my responses to this dm-devel thread, and your private messages, have primarily taken issue with how you've chosen to conduct yourself. I'm not aware of dm-flakey's corrupt_bio_byte being used in upstream testing frameworks (xfstests only uses flakey's basic capabilities). Any willingness to elevate dm-flakey's corrupt_bio_byte to wider use in upstream testing frameworks would have uncovered the need for your fix. You were first to notice it. Rather than be cool about it, you've been hostile from the start and completely misrepresented the significance of the bug given the limited scope of who is impacted. I really am done responding to your escalating campaign of self-owns.
diff --git a/drivers/md/dm-flakey.c b/drivers/md/dm-flakey.c index 731467d4ed10..5ceb5d68f65c 100644 --- a/drivers/md/dm-flakey.c +++ b/drivers/md/dm-flakey.c @@ -426,7 +430,7 @@ static struct bio *clone_bio(struct dm_target *ti, struct flakey_c *fc, struct b if (!clone) return NULL; - bio_init(clone, fc->dev->bdev, bio->bi_inline_vecs, nr_iovecs, bio->bi_opf); + bio_init(clone, fc->dev->bdev, clone->bi_inline_vecs, nr_iovecs, bio->bi_opf); clone->bi_iter.bi_sector = flakey_map_sector(ti, bio->bi_iter.bi_sector); clone->bi_private = bio;
So, this code clearly isn't getting tested - at all. Besides this bug, the parsing for the "corrupt" modes is also broken. Guys, don't push broken crap, and figure out how to write some tests. -- >8 -- Cc: dm-devel@lists.linux.dev Cc: Mikulas Patocka <mpatocka@redhat.com> Cc: Mike Snitzer <snitzer@kernel.org> Cc: Alasdair Kergon <agk@redhat.com> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> --- drivers/md/dm-flakey.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)