| Message ID | 20180824211535.GA22251@beast (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Replace bio_check_ro()'s WARN_ON() | expand |
Friendly ping! I keep tripping over this. Can this please get applied so we can silence syzbot and avoid needless WARNs? :) -Kees On Fri, Aug 24, 2018 at 02:15:35PM -0700, Kees Cook wrote: > As described in commit 96c6a32ccb55a ("include/asm-generic/bug.h: clarify > valid uses of WARN()"), this replaces a userspace-reachable WARN_ON() > with pr_warn_once(). The reachability is even noted in the existing > comment. This appears to be an "expected by unlikely" condition, so > getting rid of the WARN_ON() means kernel fuzzers will stop reporting > the problem. Additionally un-breaks the error string so it can more > easily be found with grep. > > Reported-by: syzbot+21cfe1f803e0e158acf1@syzkaller.appspotmail.com > Cc: Jens Axboe <axboe@kernel.dk> > Cc: linux-block@vger.kernel.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > block/blk-core.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index dee56c282efb..470c3cea8cb0 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -2166,11 +2166,9 @@ static inline bool bio_check_ro(struct bio *bio, struct hd_struct *part) > if (part->policy && (op_is_write(op) && !op_is_flush(op))) { > char b[BDEVNAME_SIZE]; > > - WARN_ONCE(1, > - "generic_make_request: Trying to write " > - "to read-only block-device %s (partno %d)\n", > + /* Older lvm-tools actually triggers this. */ > + pr_warn_once("Trying to write to read-only block-device %s (partno %d)\n", > bio_devname(bio, b), part->partno); > - /* Older lvm-tools actually trigger this */ > return false; > } > > -- > 2.17.1 > > > -- > Kees Cook > Pixel Security
On 11/22/19 11:53 AM, Kees Cook wrote: > Friendly ping! I keep tripping over this. Can this please get applied so > we can silence syzbot and avoid needless WARNs? :) I'll get it applied, I did see syzbot complain about this again.
On Fri, Nov 22, 2019 at 11:55:11AM -0700, Jens Axboe wrote: > On 11/22/19 11:53 AM, Kees Cook wrote: > > Friendly ping! I keep tripping over this. Can this please get applied so > > we can silence syzbot and avoid needless WARNs? :) > > I'll get it applied, I did see syzbot complain about this again. Awesome; thanks! :)
On Fri, Nov 22, 2019 at 10:53:22AM -0800, Kees Cook wrote: > Friendly ping! I keep tripping over this. Can this please get applied so > we can silence syzbot and avoid needless WARNs? :) What call stack reaches this? Upper layers should never submit a write bio on a read-only queue, and we need to fix that in the upper layer.
On 11/22/19 12:07 PM, Christoph Hellwig wrote: > On Fri, Nov 22, 2019 at 10:53:22AM -0800, Kees Cook wrote: >> Friendly ping! I keep tripping over this. Can this please get applied so >> we can silence syzbot and avoid needless WARNs? :) > > What call stack reaches this? Upper layers should never submit a write > bio on a read-only queue, and we need to fix that in the upper layer. It's an fsync, the trace is here: https://syzkaller.appspot.com/x/log.txt?x=159503d2e00000
On Fri, Nov 22, 2019 at 12:09:14PM -0700, Jens Axboe wrote: > On 11/22/19 12:07 PM, Christoph Hellwig wrote: > > On Fri, Nov 22, 2019 at 10:53:22AM -0800, Kees Cook wrote: > >> Friendly ping! I keep tripping over this. Can this please get applied so > >> we can silence syzbot and avoid needless WARNs? :) > > > > What call stack reaches this? Upper layers should never submit a write > > bio on a read-only queue, and we need to fix that in the upper layer. > > It's an fsync, the trace is here: > > https://syzkaller.appspot.com/x/log.txt?x=159503d2e00000 Oh. I think this is a bug in the block layer, we should not treat a sync as write for the purposes of is read-only checks, as it never writes data to the device. At the request layer we alread use the proper REQ_OP_FLUSH, but at the bio layer we are still abusing empty writes apparently. I'll try to cook up something over the weekend.
On Fri, Nov 22, 2019 at 11:14:34AM -0800, Christoph Hellwig wrote: > On Fri, Nov 22, 2019 at 12:09:14PM -0700, Jens Axboe wrote: > > On 11/22/19 12:07 PM, Christoph Hellwig wrote: > > > On Fri, Nov 22, 2019 at 10:53:22AM -0800, Kees Cook wrote: > > >> Friendly ping! I keep tripping over this. Can this please get applied so > > >> we can silence syzbot and avoid needless WARNs? :) > > > > > > What call stack reaches this? Upper layers should never submit a write > > > bio on a read-only queue, and we need to fix that in the upper layer. > > > > It's an fsync, the trace is here: > > > > https://syzkaller.appspot.com/x/log.txt?x=159503d2e00000 > > Oh. I think this is a bug in the block layer, we should not treat > a sync as write for the purposes of is read-only checks, as it never > writes data to the device. At the request layer we alread use > the proper REQ_OP_FLUSH, but at the bio layer we are still abusing > empty writes apparently. I'll try to cook up something over the > weekend. Cool; thanks! Note that syzbot has a reproducer for it: https://syzkaller.appspot.com/text?tag=ReproC&x=117ccc8c400000 If that doesn't work for your own testing, you can ask syzbot to test patches itself: https://goo.gl/tpsmEJ#testing-patches
So I looked at this a bit, and doing the right thing (TM) will be
a little invase and thus not for 5.5.
But the 5.5. queue already has a patch from Mikulas Patocka:
8b2ded1c94c ("block: don't warn when doing fsync on read-only devices")
which should deal with this issue, and in fact I can't trigger the
WARN_ON with Jens' latest tree.
diff --git a/block/blk-core.c b/block/blk-core.c index dee56c282efb..470c3cea8cb0 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -2166,11 +2166,9 @@ static inline bool bio_check_ro(struct bio *bio, struct hd_struct *part) if (part->policy && (op_is_write(op) && !op_is_flush(op))) { char b[BDEVNAME_SIZE]; - WARN_ONCE(1, - "generic_make_request: Trying to write " - "to read-only block-device %s (partno %d)\n", + /* Older lvm-tools actually triggers this. */ + pr_warn_once("Trying to write to read-only block-device %s (partno %d)\n", bio_devname(bio, b), part->partno); - /* Older lvm-tools actually trigger this */ return false; }
As described in commit 96c6a32ccb55a ("include/asm-generic/bug.h: clarify valid uses of WARN()"), this replaces a userspace-reachable WARN_ON() with pr_warn_once(). The reachability is even noted in the existing comment. This appears to be an "expected by unlikely" condition, so getting rid of the WARN_ON() means kernel fuzzers will stop reporting the problem. Additionally un-breaks the error string so it can more easily be found with grep. Reported-by: syzbot+21cfe1f803e0e158acf1@syzkaller.appspotmail.com Cc: Jens Axboe <axboe@kernel.dk> Cc: linux-block@vger.kernel.org Signed-off-by: Kees Cook <keescook@chromium.org> --- block/blk-core.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)