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