* [PATCH] direct-io: prevent possible race condition on bio_list
@ 2022-02-26 22:17 Niels Dossche
2022-02-26 22:25 ` Matthew Wilcox
0 siblings, 1 reply; 4+ messages in thread
From: Niels Dossche @ 2022-02-26 22:17 UTC (permalink / raw)
To: linux-fsdevel; +Cc: Alexander Viro, Niels Dossche
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(-)
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] direct-io: prevent possible race condition on bio_list
2022-02-26 22:17 [PATCH] direct-io: prevent possible race condition on bio_list Niels Dossche
@ 2022-02-26 22:25 ` Matthew Wilcox
2022-02-26 22:29 ` Niels Dossche
0 siblings, 1 reply; 4+ messages in thread
From: Matthew Wilcox @ 2022-02-26 22:25 UTC (permalink / raw)
To: Niels Dossche; +Cc: linux-fsdevel, Alexander Viro
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] direct-io: prevent possible race condition on bio_list
2022-02-26 22:25 ` Matthew Wilcox
@ 2022-02-26 22:29 ` Niels Dossche
2022-04-28 15:00 ` Jan Kara
0 siblings, 1 reply; 4+ messages in thread
From: Niels Dossche @ 2022-02-26 22:29 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: linux-fsdevel, Alexander Viro
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
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] direct-io: prevent possible race condition on bio_list
2022-02-26 22:29 ` Niels Dossche
@ 2022-04-28 15:00 ` Jan Kara
0 siblings, 0 replies; 4+ messages in thread
From: Jan Kara @ 2022-04-28 15:00 UTC (permalink / raw)
To: Niels Dossche; +Cc: Matthew Wilcox, linux-fsdevel, Alexander Viro
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
> >>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-28 15:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-26 22:17 [PATCH] direct-io: prevent possible race condition on bio_list Niels Dossche
2022-02-26 22:25 ` Matthew Wilcox
2022-02-26 22:29 ` Niels Dossche
2022-04-28 15:00 ` Jan Kara
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.