| Message ID | 20220226221748.197800-1-dossche.niels@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | direct-io: prevent possible race condition on bio_list | expand |
On Sat, Feb 26, 2022 at 11:17:48PM +0100, Niels Dossche wrote: > Prevent bio_list from changing in the while loop condition such that the > body of the loop won't execute with a potentially NULL pointer for > bio_list, which causes a NULL dereference later on. Is this something you've seen happen, or something you think might happen? > Signed-off-by: Niels Dossche <dossche.niels@gmail.com> > --- > fs/direct-io.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/fs/direct-io.c b/fs/direct-io.c > index 654443558047..806f05407019 100644 > --- a/fs/direct-io.c > +++ b/fs/direct-io.c > @@ -545,19 +545,22 @@ static inline int dio_bio_reap(struct dio *dio, struct dio_submit *sdio) > int ret = 0; > > if (sdio->reap_counter++ >= 64) { > + unsigned long flags; > + > + spin_lock_irqsave(&dio->bio_lock, flags); > while (dio->bio_list) { > - unsigned long flags; > struct bio *bio; > int ret2; > > - spin_lock_irqsave(&dio->bio_lock, flags); > bio = dio->bio_list; > dio->bio_list = bio->bi_private; > spin_unlock_irqrestore(&dio->bio_lock, flags); > ret2 = blk_status_to_errno(dio_bio_complete(dio, bio)); > if (ret == 0) > ret = ret2; > + spin_lock_irqsave(&dio->bio_lock, flags); > } > + spin_unlock_irqrestore(&dio->bio_lock, flags); > sdio->reap_counter = 0; > } > return ret; > -- > 2.35.1 >
On 2/26/22 23:25, Matthew Wilcox wrote: > On Sat, Feb 26, 2022 at 11:17:48PM +0100, Niels Dossche wrote: >> Prevent bio_list from changing in the while loop condition such that the >> body of the loop won't execute with a potentially NULL pointer for >> bio_list, which causes a NULL dereference later on. > > Is this something you've seen happen, or something you think might > happen? > This is something that I think might happen, not something I've seen. >> Signed-off-by: Niels Dossche <dossche.niels@gmail.com> >> --- >> fs/direct-io.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/fs/direct-io.c b/fs/direct-io.c >> index 654443558047..806f05407019 100644 >> --- a/fs/direct-io.c >> +++ b/fs/direct-io.c >> @@ -545,19 +545,22 @@ static inline int dio_bio_reap(struct dio *dio, struct dio_submit *sdio) >> int ret = 0; >> >> if (sdio->reap_counter++ >= 64) { >> + unsigned long flags; >> + >> + spin_lock_irqsave(&dio->bio_lock, flags); >> while (dio->bio_list) { >> - unsigned long flags; >> struct bio *bio; >> int ret2; >> >> - spin_lock_irqsave(&dio->bio_lock, flags); >> bio = dio->bio_list; >> dio->bio_list = bio->bi_private; >> spin_unlock_irqrestore(&dio->bio_lock, flags); >> ret2 = blk_status_to_errno(dio_bio_complete(dio, bio)); >> if (ret == 0) >> ret = ret2; >> + spin_lock_irqsave(&dio->bio_lock, flags); >> } >> + spin_unlock_irqrestore(&dio->bio_lock, flags); >> sdio->reap_counter = 0; >> } >> return ret; >> -- >> 2.35.1 >>
On Sat 26-02-22 23:29:03, Niels Dossche wrote: > On 2/26/22 23:25, Matthew Wilcox wrote: > > On Sat, Feb 26, 2022 at 11:17:48PM +0100, Niels Dossche wrote: > >> Prevent bio_list from changing in the while loop condition such that the > >> body of the loop won't execute with a potentially NULL pointer for > >> bio_list, which causes a NULL dereference later on. > > > > Is this something you've seen happen, or something you think might > > happen? > > > > This is something that I think might happen, not something I've seen. I can see this didn't get merged. I agree the code looks fishy but AFAICT it is safe. The reason is that the only code that can currently remove bio from bio_list is under dio_await_completion() which cannot run concurrently with dio_bio_reap() on the same bio... It might deserve a comment though. Honza > > >> Signed-off-by: Niels Dossche <dossche.niels@gmail.com> > >> --- > >> fs/direct-io.c | 7 +++++-- > >> 1 file changed, 5 insertions(+), 2 deletions(-) > >> > >> diff --git a/fs/direct-io.c b/fs/direct-io.c > >> index 654443558047..806f05407019 100644 > >> --- a/fs/direct-io.c > >> +++ b/fs/direct-io.c > >> @@ -545,19 +545,22 @@ static inline int dio_bio_reap(struct dio *dio, struct dio_submit *sdio) > >> int ret = 0; > >> > >> if (sdio->reap_counter++ >= 64) { > >> + unsigned long flags; > >> + > >> + spin_lock_irqsave(&dio->bio_lock, flags); > >> while (dio->bio_list) { > >> - unsigned long flags; > >> struct bio *bio; > >> int ret2; > >> > >> - spin_lock_irqsave(&dio->bio_lock, flags); > >> bio = dio->bio_list; > >> dio->bio_list = bio->bi_private; > >> spin_unlock_irqrestore(&dio->bio_lock, flags); > >> ret2 = blk_status_to_errno(dio_bio_complete(dio, bio)); > >> if (ret == 0) > >> ret = ret2; > >> + spin_lock_irqsave(&dio->bio_lock, flags); > >> } > >> + spin_unlock_irqrestore(&dio->bio_lock, flags); > >> sdio->reap_counter = 0; > >> } > >> return ret; > >> -- > >> 2.35.1 > >>
diff --git a/fs/direct-io.c b/fs/direct-io.c index 654443558047..806f05407019 100644 --- a/fs/direct-io.c +++ b/fs/direct-io.c @@ -545,19 +545,22 @@ static inline int dio_bio_reap(struct dio *dio, struct dio_submit *sdio) int ret = 0; if (sdio->reap_counter++ >= 64) { + unsigned long flags; + + spin_lock_irqsave(&dio->bio_lock, flags); while (dio->bio_list) { - unsigned long flags; struct bio *bio; int ret2; - spin_lock_irqsave(&dio->bio_lock, flags); bio = dio->bio_list; dio->bio_list = bio->bi_private; spin_unlock_irqrestore(&dio->bio_lock, flags); ret2 = blk_status_to_errno(dio_bio_complete(dio, bio)); if (ret == 0) ret = ret2; + spin_lock_irqsave(&dio->bio_lock, flags); } + spin_unlock_irqrestore(&dio->bio_lock, flags); sdio->reap_counter = 0; } return ret;
Prevent bio_list from changing in the while loop condition such that the body of the loop won't execute with a potentially NULL pointer for bio_list, which causes a NULL dereference later on. Signed-off-by: Niels Dossche <dossche.niels@gmail.com> --- fs/direct-io.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)