Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Markus Elfring]

> … released the refcount of the bdev (actually the refcount of
> the bdev inode).

Wording adjustments:
… released the reference count of the block device inode.


> … access bdev after …

… access block device after …


> accually bdev is …

bdev is …


> … This may leads to use-after-free if the refcount is …

… This results in an use after free if the reference count …


> following scenerio:

following scenario:


Would you like to add the tag “Fixes” to the commit message?

Regards,
Markus

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Markus Elfring]

> … released the refcount of the bdev (actually the refcount of
> the bdev inode).

Wording adjustments:
… released the reference count of the block device inode.


> … access bdev after …

… access block device after …


> accually bdev is …

bdev is …


> … This may leads to use-after-free if the refcount is …

… This results in an use after free if the reference count …


> following scenerio:

following scenario:


Would you like to add the tag “Fixes” to the commit message?

Regards,
Markus

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Jason Yan]

Hi, Markus

Thanks for the review.

Sorry for the wording because I'm not an English native speaker.

在 2020/6/5 16:30, Markus Elfring 写道:

> 
> Would you like to add the tag “Fixes” to the commit message?
> 

I tried to find the commit in the git history which introduced this 
issue, but I am not sure which one is it. It seems existed there for a 
long time.

Thanks,
Jason


> Regards,
> Markus
> 

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Jason Yan]

Hi, Markus

Thanks for the review.

Sorry for the wording because I'm not an English native speaker.

在 2020/6/5 16:30, Markus Elfring 写道:

> 
> Would you like to add the tag “Fixes” to the commit message?
> 

I tried to find the commit in the git history which introduced this 
issue, but I am not sure which one is it. It seems existed there for a 
long time.

Thanks,
Jason


> Regards,
> Markus
> 

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Dan Carpenter]

A lot of maintainers have blocked Markus and asked him to stop trying
to help people write commit message.  Saying "bdev" instead of "block
device" is more clear so your original message was better.

The Fixes tag is a good idea though:

Fixes: 89e524c04fa9 ("loop: Fix mount(2) failure due to race with LOOP_SET_FD")

It broke last July.  Before that, we used to check if __blkdev_get()
failed before dereferencing "bdev".

I wonder if maybe the best fix is to re-add the "if (!res) " check back
to blkdev_get().  The __blkdev_get() looks like it can also free "whole"
though if it calls itself recursively and I don't really know this code
so I can't say for sure...

regards,
dan carpenter

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Dan Carpenter]

A lot of maintainers have blocked Markus and asked him to stop trying
to help people write commit message.  Saying "bdev" instead of "block
device" is more clear so your original message was better.

The Fixes tag is a good idea though:

Fixes: 89e524c04fa9 ("loop: Fix mount(2) failure due to race with LOOP_SET_FD")

It broke last July.  Before that, we used to check if __blkdev_get()
failed before dereferencing "bdev".

I wonder if maybe the best fix is to re-add the "if (!res) " check back
to blkdev_get().  The __blkdev_get() looks like it can also free "whole"
though if it calls itself recursively and I don't really know this code
so I can't say for sure...

regards,
dan carpenter

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Markus Elfring]

> A lot of maintainers have blocked Markus and asked him to stop trying
> to help people write commit message.

I am trying to contribute a bit of patch review as usual.


> Saying "bdev" instead of "block device" is more clear

I find this view interesting.


> so your original message was better.

Does this feedback include reported spelling mistakes?


> The Fixes tag is a good idea though:
>
> Fixes: 89e524c04fa9 ("loop: Fix mount(2) failure due to race with LOOP_SET_FD")

Thanks for your addition.

Regards,
Markus

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Markus Elfring]

> A lot of maintainers have blocked Markus and asked him to stop trying
> to help people write commit message.

I am trying to contribute a bit of patch review as usual.


> Saying "bdev" instead of "block device" is more clear

I find this view interesting.


> so your original message was better.

Does this feedback include reported spelling mistakes?


> The Fixes tag is a good idea though:
>
> Fixes: 89e524c04fa9 ("loop: Fix mount(2) failure due to race with LOOP_SET_FD")

Thanks for your addition.

Regards,
Markus

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Dan Carpenter]

On Fri, Jun 05, 2020 at 12:56:45PM +0200, Markus Elfring wrote:
> > A lot of maintainers have blocked Markus and asked him to stop trying
> > to help people write commit message.
> 
> I am trying to contribute a bit of patch review as usual.
> 

We have asked you again and again to stop commenting on commit messages.
New kernel developers have emailed me privately to say that your review
comments confused and discouraged them.  Greg has created a email bot to
respond to your commit message reviews.

regards,
dan carpenter

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Dan Carpenter]

On Fri, Jun 05, 2020 at 12:56:45PM +0200, Markus Elfring wrote:
> > A lot of maintainers have blocked Markus and asked him to stop trying
> > to help people write commit message.
> 
> I am trying to contribute a bit of patch review as usual.
> 

We have asked you again and again to stop commenting on commit messages.
New kernel developers have emailed me privately to say that your review
comments confused and discouraged them.  Greg has created a email bot to
respond to your commit message reviews.

regards,
dan carpenter

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Matthew Wilcox]

On Fri, Jun 05, 2020 at 12:56:45PM +0200, Markus Elfring wrote:
> > A lot of maintainers have blocked Markus and asked him to stop trying
> > to help people write commit message.
> 
> I am trying to contribute a bit of patch review as usual.

Please stop criticising people's commit messages.  Your suggestions
are usually not improvements.

> > Saying "bdev" instead of "block device" is more clear
> 
> I find this view interesting.

Dan is right.

> > so your original message was better.
> 
> Does this feedback include reported spelling mistakes?

You can keep doing that.  But refcount -> reference count is not
particularly interesting.

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Matthew Wilcox]

On Fri, Jun 05, 2020 at 12:56:45PM +0200, Markus Elfring wrote:
> > A lot of maintainers have blocked Markus and asked him to stop trying
> > to help people write commit message.
> 
> I am trying to contribute a bit of patch review as usual.

Please stop criticising people's commit messages.  Your suggestions
are usually not improvements.

> > Saying "bdev" instead of "block device" is more clear
> 
> I find this view interesting.

Dan is right.

> > so your original message was better.
> 
> Does this feedback include reported spelling mistakes?

You can keep doing that.  But refcount -> reference count is not
particularly interesting.

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Sedat Dilek]

On Fri, Jun 5, 2020 at 11:46 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> A lot of maintainers have blocked Markus and asked him to stop trying
> to help people write commit message.  Saying "bdev" instead of "block
> device" is more clear so your original message was better.
>
> The Fixes tag is a good idea though:
>
> Fixes: 89e524c04fa9 ("loop: Fix mount(2) failure due to race with LOOP_SET_FD")
>
> It broke last July.  Before that, we used to check if __blkdev_get()
> failed before dereferencing "bdev".
>
> I wonder if maybe the best fix is to re-add the "if (!res) " check back
> to blkdev_get().  The __blkdev_get() looks like it can also free "whole"
> though if it calls itself recursively and I don't really know this code
> so I can't say for sure...
>

In things of Fixes: tag...

For the first hunk I found:

commit 8266602033d6adc6d10cb8811c1fd694767909b0 ("fix bdev leak in
block_dev.c do_open()")

- Sedat -

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Sedat Dilek]

On Fri, Jun 5, 2020 at 11:46 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> A lot of maintainers have blocked Markus and asked him to stop trying
> to help people write commit message.  Saying "bdev" instead of "block
> device" is more clear so your original message was better.
>
> The Fixes tag is a good idea though:
>
> Fixes: 89e524c04fa9 ("loop: Fix mount(2) failure due to race with LOOP_SET_FD")
>
> It broke last July.  Before that, we used to check if __blkdev_get()
> failed before dereferencing "bdev".
>
> I wonder if maybe the best fix is to re-add the "if (!res) " check back
> to blkdev_get().  The __blkdev_get() looks like it can also free "whole"
> though if it calls itself recursively and I don't really know this code
> so I can't say for sure...
>

In things of Fixes: tag...

For the first hunk I found:

commit 8266602033d6adc6d10cb8811c1fd694767909b0 ("fix bdev leak in
block_dev.c do_open()")

- Sedat -

Re: block: Fix use-after-free in blkdev_get()

[Markus Elfring]

>> I am trying to contribute a bit of patch review as usual.
>
> We have asked you again and again to stop commenting on commit messages.

I am going to continue with constructive feedback at some places.


> New kernel developers have emailed me privately to say that your review
> comments confused and discouraged them.

Did these contributors not dare to ask me directly about mentioned details?

Some developers found parts of my reviews helpful, didn't they?

Regards,
Markus

Re: block: Fix use-after-free in blkdev_get()

[Markus Elfring]

>> I am trying to contribute a bit of patch review as usual.
>
> We have asked you again and again to stop commenting on commit messages.

I am going to continue with constructive feedback at some places.


> New kernel developers have emailed me privately to say that your review
> comments confused and discouraged them.

Did these contributors not dare to ask me directly about mentioned details?

Some developers found parts of my reviews helpful, didn't they?

Regards,
Markus

Re: block: Fix use-after-free in blkdev_get()

[Matthew Wilcox]

On Fri, Jun 05, 2020 at 01:40:37PM +0200, Markus Elfring wrote:
> >> I am trying to contribute a bit of patch review as usual.
> >
> > We have asked you again and again to stop commenting on commit messages.
> 
> I am going to continue with constructive feedback at some places.
> 
> 
> > New kernel developers have emailed me privately to say that your review
> > comments confused and discouraged them.
> 
> Did these contributors not dare to ask me directly about mentioned details?
> 
> Some developers found parts of my reviews helpful, didn't they?

Overall you are a net negative to kernel development.  Please change
how you contribute.

Re: block: Fix use-after-free in blkdev_get()

[Matthew Wilcox]

On Fri, Jun 05, 2020 at 01:40:37PM +0200, Markus Elfring wrote:
> >> I am trying to contribute a bit of patch review as usual.
> >
> > We have asked you again and again to stop commenting on commit messages.
> 
> I am going to continue with constructive feedback at some places.
> 
> 
> > New kernel developers have emailed me privately to say that your review
> > comments confused and discouraged them.
> 
> Did these contributors not dare to ask me directly about mentioned details?
> 
> Some developers found parts of my reviews helpful, didn't they?

Overall you are a net negative to kernel development.  Please change
how you contribute.

Re: block: Fix use-after-free in blkdev_get()

[Markus Elfring]

>> I am trying to contribute a bit of patch review as usual.
>
> Please stop criticising people's commit messages.  Your suggestions
> are usually not improvements.

The details can vary also for my suggestions.
Would you point any more disagreemnents out on concrete items?


> But refcount -> reference count is not particularly interesting.

Can a wording clarification become helpful also for this issue?

Regards,
Markus

Re: block: Fix use-after-free in blkdev_get()

[Markus Elfring]

>> I am trying to contribute a bit of patch review as usual.
>
> Please stop criticising people's commit messages.  Your suggestions
> are usually not improvements.

The details can vary also for my suggestions.
Would you point any more disagreemnents out on concrete items?


> But refcount -> reference count is not particularly interesting.

Can a wording clarification become helpful also for this issue?

Regards,
Markus

Re: block: Fix use-after-free in blkdev_get()

[Matthew Wilcox]

On Fri, Jun 05, 2020 at 01:48:43PM +0200, Markus Elfring wrote:
> >> I am trying to contribute a bit of patch review as usual.
> >
> > Please stop criticising people's commit messages.  Your suggestions
> > are usually not improvements.
> 
> The details can vary also for my suggestions.
> Would you point any more disagreemnents out on concrete items?

That's exactly the problem with many of your comments.  They're
vague to the point of unintelligibility.

> > But refcount -> reference count is not particularly interesting.
> 
> Can a wording clarification become helpful also for this issue?

This is a great example.  I have no idea what this sentence means.
I speak some German; how would you say this in German?

Re: block: Fix use-after-free in blkdev_get()

[Matthew Wilcox]

On Fri, Jun 05, 2020 at 01:48:43PM +0200, Markus Elfring wrote:
> >> I am trying to contribute a bit of patch review as usual.
> >
> > Please stop criticising people's commit messages.  Your suggestions
> > are usually not improvements.
> 
> The details can vary also for my suggestions.
> Would you point any more disagreemnents out on concrete items?

That's exactly the problem with many of your comments.  They're
vague to the point of unintelligibility.

> > But refcount -> reference count is not particularly interesting.
> 
> Can a wording clarification become helpful also for this issue?

This is a great example.  I have no idea what this sentence means.
I speak some German; how would you say this in German?

Re: block: Fix use-after-free in blkdev_get()

[Markus Elfring]

>> Some developers found parts of my reviews helpful, didn't they?
>
> Overall you are a net negative to kernel development.

Which concrete items do you like less here?


> Please change how you contribute.

I am curious to find the details out which might hinder progress
in desirable directions (according to your views)?

Regards,
Markus

Re: block: Fix use-after-free in blkdev_get()

[Markus Elfring]

>> Some developers found parts of my reviews helpful, didn't they?
>
> Overall you are a net negative to kernel development.

Which concrete items do you like less here?


> Please change how you contribute.

I am curious to find the details out which might hinder progress
in desirable directions (according to your views)?

Regards,
Markus

Re: block: Fix use-after-free in blkdev_get()

[Matthew Wilcox]

On Fri, Jun 05, 2020 at 02:47:00PM +0200, Markus Elfring wrote:
> >> Some developers found parts of my reviews helpful, didn't they?
> >
> > Overall you are a net negative to kernel development.
> 
> Which concrete items do you like less here?

Your feedback is unhelpful and you show no signs of changing it in
response to the people who are telling you that it's unhelpful.

> > Please change how you contribute.
> 
> I am curious to find the details out which might hinder progress
> in desirable directions (according to your views)?

Become an expert at something.  You seem to know about a millimetre deep
across many hectares.  Learn something deeply, then your opinion about
it will be meaningful.

Re: block: Fix use-after-free in blkdev_get()

[Matthew Wilcox]

On Fri, Jun 05, 2020 at 02:47:00PM +0200, Markus Elfring wrote:
> >> Some developers found parts of my reviews helpful, didn't they?
> >
> > Overall you are a net negative to kernel development.
> 
> Which concrete items do you like less here?

Your feedback is unhelpful and you show no signs of changing it in
response to the people who are telling you that it's unhelpful.

> > Please change how you contribute.
> 
> I am curious to find the details out which might hinder progress
> in desirable directions (according to your views)?

Become an expert at something.  You seem to know about a millimetre deep
across many hectares.  Learn something deeply, then your opinion about
it will be meaningful.

Re: block: Fix use-after-free in blkdev_get()

[Markus Elfring]

>> The details can vary also for my suggestions.
>> Would you point any more disagreements out on concrete items?
>
> That's exactly the problem with many of your comments.
> They're vague to the point of unintelligibility.

Was is so vague about possibilities which I point out for patch reviews
(for example)?
* Spelling corrections
* Additional wording alternatives


>>> But refcount -> reference count is not particularly interesting.
>>
>> Can a wording clarification become helpful also for this issue?
>
> This is a great example.  I have no idea what this sentence means.

Some developers usually prefer to use abbreviations at specific places
while I dare to propose the usage of another well-known term
for commit messages.

Regards,
Markus

Re: block: Fix use-after-free in blkdev_get()

[Markus Elfring]

>> The details can vary also for my suggestions.
>> Would you point any more disagreements out on concrete items?
>
> That's exactly the problem with many of your comments.
> They're vague to the point of unintelligibility.

Was is so vague about possibilities which I point out for patch reviews
(for example)?
* Spelling corrections
* Additional wording alternatives


>>> But refcount -> reference count is not particularly interesting.
>>
>> Can a wording clarification become helpful also for this issue?
>
> This is a great example.  I have no idea what this sentence means.

Some developers usually prefer to use abbreviations at specific places
while I dare to propose the usage of another well-known term
for commit messages.

Regards,
Markus

Re: block: Fix use-after-free in blkdev_get()

[Markus Elfring]

>> The details can vary also for my suggestions.
>> Would you point any more disagreements out on concrete items?
>
> That's exactly the problem with many of your comments.
> They're vague to the point of unintelligibility.

Was is so vague about possibilities which I point out for patch reviews
(for example)?
* Spelling corrections
* Additional wording alternatives


>>> But refcount -> reference count is not particularly interesting.
>>
>> Can a wording clarification become helpful also for this issue?
>
> This is a great example.  I have no idea what this sentence means.

Some developers usually prefer to use abbreviations at specific places
while I dare to propose the usage of another well-known term
for commit messages.

Regards,
Markus

Re: block: Fix use-after-free in blkdev_get()

[Markus Elfring]

>> The details can vary also for my suggestions.
>> Would you point any more disagreements out on concrete items?
>
> That's exactly the problem with many of your comments.
> They're vague to the point of unintelligibility.

Was is so vague about possibilities which I point out for patch reviews
(for example)?
* Spelling corrections
* Additional wording alternatives


>>> But refcount -> reference count is not particularly interesting.
>>
>> Can a wording clarification become helpful also for this issue?
>
> This is a great example.  I have no idea what this sentence means.

Some developers usually prefer to use abbreviations at specific places
while I dare to propose the usage of another well-known term
for commit messages.

Regards,
Markus

Re: block: Fix use-after-free in blkdev_get()

[Jens Axboe]

On 6/5/20 7:01 AM, Markus Elfring wrote:
>>> The details can vary also for my suggestions.
>>> Would you point any more disagreements out on concrete items?
>>
>> That's exactly the problem with many of your comments.
>> They're vague to the point of unintelligibility.
> 
> Was is so vague about possibilities which I point out for patch reviews
> (for example)?
> * Spelling corrections
> * Additional wording alternatives

I'll make this very simple - don't reply making suggestions to commit
messages, ever. The "improvements" aren't usually improvements, and it
causes more confusion for the submitter.

Maintainers generally do change commit messages to improve them, if
needed.

No reply is needed to this e-mail.

-- 
Jens Axboe

Re: block: Fix use-after-free in blkdev_get()

[Jens Axboe]

On 6/5/20 7:01 AM, Markus Elfring wrote:
>>> The details can vary also for my suggestions.
>>> Would you point any more disagreements out on concrete items?
>>
>> That's exactly the problem with many of your comments.
>> They're vague to the point of unintelligibility.
> 
> Was is so vague about possibilities which I point out for patch reviews
> (for example)?
> * Spelling corrections
> * Additional wording alternatives

I'll make this very simple - don't reply making suggestions to commit
messages, ever. The "improvements" aren't usually improvements, and it
causes more confusion for the submitter.

Maintainers generally do change commit messages to improve them, if
needed.

No reply is needed to this e-mail.

-- 
Jens Axboe

Re: block: Fix use-after-free in blkdev_get()

[Markus Elfring]

> Your feedback is unhelpful

Do you find proposed spelling corrections useful?


> and you show no signs of changing it in response to the people
> who are telling you that it's unhelpful.

Other adjustments can occasionally be more challenging
besides the usual communication challenges.

Regards,
Markus

Re: block: Fix use-after-free in blkdev_get()

[Markus Elfring]

> Your feedback is unhelpful

Do you find proposed spelling corrections useful?


> and you show no signs of changing it in response to the people
> who are telling you that it's unhelpful.

Other adjustments can occasionally be more challenging
besides the usual communication challenges.

Regards,
Markus

Re: block: Fix use-after-free in blkdev_get()

[Markus Elfring]

> Maintainers generally do change commit messages to improve them,
> if needed.

You have got a documented choice here.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=435faf5c218a47fd6258187f62d9bb1009717896#n468

Regards,
Markus

Re: block: Fix use-after-free in blkdev_get()

[Markus Elfring]

> Maintainers generally do change commit messages to improve them,
> if needed.

You have got a documented choice here.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?idC5faf5c218a47fd6258187f62d9bb1009717896#n468

Regards,
Markus

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Jan Kara]

On Fri 05-06-20 12:43:54, Dan Carpenter wrote:
> I wonder if maybe the best fix is to re-add the "if (!res) " check back
> to blkdev_get().

Well, it won't be that simple since we need to call bd_abort_claiming()
under bdev->bd_mutex. And the fact that __blkdev_get() frees the reference
you pass to it is somewhat subtle and surprising so I think we are better
off getting rid of that.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Jan Kara]

On Fri 05-06-20 12:43:54, Dan Carpenter wrote:
> I wonder if maybe the best fix is to re-add the "if (!res) " check back
> to blkdev_get().

Well, it won't be that simple since we need to call bd_abort_claiming()
under bdev->bd_mutex. And the fact that __blkdev_get() frees the reference
you pass to it is somewhat subtle and surprising so I think we are better
off getting rid of that.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: block: Fix use-after-free in blkdev_get()

[Matthew Wilcox]

On Fri, Jun 05, 2020 at 03:12:08PM +0200, Markus Elfring wrote:
> > Your feedback is unhelpful
> 
> Do you find proposed spelling corrections useful?

To commit messages?  No.

> > and you show no signs of changing it in response to the people
> > who are telling you that it's unhelpful.
> 
> Other adjustments can occasionally be more challenging
> besides the usual communication challenges.

I think I'm going to ask Greg if I can borrow his bot.

Many hackers start out by just making spelling fixes to code.  And that's
fine, often they progress to more substantial contributions.  You do not
seem to progress, and making spelling corrections to changelog messages
is a level of nitpicking that just isn't helpful.

Re: block: Fix use-after-free in blkdev_get()

[Matthew Wilcox]

On Fri, Jun 05, 2020 at 03:12:08PM +0200, Markus Elfring wrote:
> > Your feedback is unhelpful
> 
> Do you find proposed spelling corrections useful?

To commit messages?  No.

> > and you show no signs of changing it in response to the people
> > who are telling you that it's unhelpful.
> 
> Other adjustments can occasionally be more challenging
> besides the usual communication challenges.

I think I'm going to ask Greg if I can borrow his bot.

Many hackers start out by just making spelling fixes to code.  And that's
fine, often they progress to more substantial contributions.  You do not
seem to progress, and making spelling corrections to changelog messages
is a level of nitpicking that just isn't helpful.

Re: block: Fix use-after-free in blkdev_get()

[Markus Elfring]

>> Do you find proposed spelling corrections useful?
>
> To commit messages?  No.

Are you really going to tolerate wording weaknesses there?
https://lore.kernel.org/linux-block/20200605104558.16686-1-yanaijie@huawei.com/
https://lore.kernel.org/patchwork/patch/1252648/


> You do not seem to progress, and making spelling corrections to
> changelog messages is a level of nitpicking that just isn't helpful.

I got obviously an other software development understanding
(also in this area).

Regards,
Markus

Re: block: Fix use-after-free in blkdev_get()

[Markus Elfring]

>> Do you find proposed spelling corrections useful?
>
> To commit messages?  No.

Are you really going to tolerate wording weaknesses there?
https://lore.kernel.org/linux-block/20200605104558.16686-1-yanaijie@huawei.com/
https://lore.kernel.org/patchwork/patch/1252648/


> You do not seem to progress, and making spelling corrections to
> changelog messages is a level of nitpicking that just isn't helpful.

I got obviously an other software development understanding
(also in this area).

Regards,
Markus

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Dan Carpenter]

On Fri, Jun 05, 2020 at 04:42:36PM +0200, Jan Kara wrote:
> On Fri 05-06-20 12:43:54, Dan Carpenter wrote:
> > I wonder if maybe the best fix is to re-add the "if (!res) " check back
> > to blkdev_get().
> 
> Well, it won't be that simple since we need to call bd_abort_claiming()
> under bdev->bd_mutex. And the fact that __blkdev_get() frees the reference
> you pass to it is somewhat subtle and surprising so I think we are better
> off getting rid of that.

Fair enough.

Jason Yan sent a v3 of this patch that frees "whole".  I've looked it
over pretty close and I think it's probably correct.

(not that my opinion should count for much because I don't know this
code very well at all).

regards,
dan carpenter

Re: [PATCH v2] block: Fix use-after-free in blkdev_get()

[Dan Carpenter]

On Fri, Jun 05, 2020 at 04:42:36PM +0200, Jan Kara wrote:
> On Fri 05-06-20 12:43:54, Dan Carpenter wrote:
> > I wonder if maybe the best fix is to re-add the "if (!res) " check back
> > to blkdev_get().
> 
> Well, it won't be that simple since we need to call bd_abort_claiming()
> under bdev->bd_mutex. And the fact that __blkdev_get() frees the reference
> you pass to it is somewhat subtle and surprising so I think we are better
> off getting rid of that.

Fair enough.

Jason Yan sent a v3 of this patch that frees "whole".  I've looked it
over pretty close and I think it's probably correct.

(not that my opinion should count for much because I don't know this
code very well at all).

regards,
dan carpenter

[PATCH v2] block: Fix use-after-free in blkdev_get()

[Jason Yan]

In blkdev_get() we call __blkdev_get() to do some internal jobs and if
there is some errors in __blkdev_get(), the bdput() is called which
means we have released the refcount of the bdev (actually the refcount of
the bdev inode). This means we cannot access bdev after that point. But
accually bdev is still accessed in blkdev_get() after calling
__blkdev_get(). This may leads to use-after-free if the refcount is the
last one we released in __blkdev_get(). Let's take a look at the
following scenerio:

  CPU0            CPU1                    CPU2
blkdev_open     blkdev_open           Remove disk
                  bd_acquire
		  blkdev_get
		    __blkdev_get      del_gendisk
					bdev_unhash_inode
  bd_acquire          bdev_get_gendisk
    bd_forget           failed because of unhashed
	  bdput
	              bdput (the last one)
		        bdev_evict_inode

	  	    access bdev => use after free

[  459.350216] BUG: KASAN: use-after-free in __lock_acquire+0x24c1/0x31b0
[  459.351190] Read of size 8 at addr ffff88806c815a80 by task syz-executor.0/20132
[  459.352347]
[  459.352594] CPU: 0 PID: 20132 Comm: syz-executor.0 Not tainted 4.19.90 #2
[  459.353628] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
[  459.354947] Call Trace:
[  459.355337]  dump_stack+0x111/0x19e
[  459.355879]  ? __lock_acquire+0x24c1/0x31b0
[  459.356523]  print_address_description+0x60/0x223
[  459.357248]  ? __lock_acquire+0x24c1/0x31b0
[  459.357887]  kasan_report.cold+0xae/0x2d8
[  459.358503]  __lock_acquire+0x24c1/0x31b0
[  459.359120]  ? _raw_spin_unlock_irq+0x24/0x40
[  459.359784]  ? lockdep_hardirqs_on+0x37b/0x580
[  459.360465]  ? _raw_spin_unlock_irq+0x24/0x40
[  459.361123]  ? finish_task_switch+0x125/0x600
[  459.361812]  ? finish_task_switch+0xee/0x600
[  459.362471]  ? mark_held_locks+0xf0/0xf0
[  459.363108]  ? __schedule+0x96f/0x21d0
[  459.363716]  lock_acquire+0x111/0x320
[  459.364285]  ? blkdev_get+0xce/0xbe0
[  459.364846]  ? blkdev_get+0xce/0xbe0
[  459.365390]  __mutex_lock+0xf9/0x12a0
[  459.365948]  ? blkdev_get+0xce/0xbe0
[  459.366493]  ? bdev_evict_inode+0x1f0/0x1f0
[  459.367130]  ? blkdev_get+0xce/0xbe0
[  459.367678]  ? destroy_inode+0xbc/0x110
[  459.368261]  ? mutex_trylock+0x1a0/0x1a0
[  459.368867]  ? __blkdev_get+0x3e6/0x1280
[  459.369463]  ? bdev_disk_changed+0x1d0/0x1d0
[  459.370114]  ? blkdev_get+0xce/0xbe0
[  459.370656]  blkdev_get+0xce/0xbe0
[  459.371178]  ? find_held_lock+0x2c/0x110
[  459.371774]  ? __blkdev_get+0x1280/0x1280
[  459.372383]  ? lock_downgrade+0x680/0x680
[  459.373002]  ? lock_acquire+0x111/0x320
[  459.373587]  ? bd_acquire+0x21/0x2c0
[  459.374134]  ? do_raw_spin_unlock+0x4f/0x250
[  459.374780]  blkdev_open+0x202/0x290
[  459.375325]  do_dentry_open+0x49e/0x1050
[  459.375924]  ? blkdev_get_by_dev+0x70/0x70
[  459.376543]  ? __x64_sys_fchdir+0x1f0/0x1f0
[  459.377192]  ? inode_permission+0xbe/0x3a0
[  459.377818]  path_openat+0x148c/0x3f50
[  459.378392]  ? kmem_cache_alloc+0xd5/0x280
[  459.379016]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  459.379802]  ? path_lookupat.isra.0+0x900/0x900
[  459.380489]  ? __lock_is_held+0xad/0x140
[  459.381093]  do_filp_open+0x1a1/0x280
[  459.381654]  ? may_open_dev+0xf0/0xf0
[  459.382214]  ? find_held_lock+0x2c/0x110
[  459.382816]  ? lock_downgrade+0x680/0x680
[  459.383425]  ? __lock_is_held+0xad/0x140
[  459.384024]  ? do_raw_spin_unlock+0x4f/0x250
[  459.384668]  ? _raw_spin_unlock+0x1f/0x30
[  459.385280]  ? __alloc_fd+0x448/0x560
[  459.385841]  do_sys_open+0x3c3/0x500
[  459.386386]  ? filp_open+0x70/0x70
[  459.386911]  ? trace_hardirqs_on_thunk+0x1a/0x1c
[  459.387610]  ? trace_hardirqs_off_caller+0x55/0x1c0
[  459.388342]  ? do_syscall_64+0x1a/0x520
[  459.388930]  do_syscall_64+0xc3/0x520
[  459.389490]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  459.390248] RIP: 0033:0x416211
[  459.390720] Code: 75 14 b8 02 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83
04 19 00 00 c3 48 83 ec 08 e8 0a fa ff ff 48 89 04 24 b8 02 00 00 00 0f
   05 <48> 8b 3c 24 48 89 c2 e8 53 fa ff ff 48 89 d0 48 83 c4 08 48 3d
      01
[  459.393483] RSP: 002b:00007fe45dfe9a60 EFLAGS: 00000293 ORIG_RAX: 0000000000000002
[  459.394610] RAX: ffffffffffffffda RBX: 00007fe45dfea6d4 RCX: 0000000000416211
[  459.395678] RDX: 00007fe45dfe9b0a RSI: 0000000000000002 RDI: 00007fe45dfe9b00
[  459.396758] RBP: 000000000076bf20 R08: 0000000000000000 R09: 000000000000000a
[  459.397930] R10: 0000000000000075 R11: 0000000000000293 R12: 00000000ffffffff
[  459.399022] R13: 0000000000000bd9 R14: 00000000004cdb80 R15: 000000000076bf2c
[  459.400168]
[  459.400430] Allocated by task 20132:
[  459.401038]  kasan_kmalloc+0xbf/0xe0
[  459.401652]  kmem_cache_alloc+0xd5/0x280
[  459.402330]  bdev_alloc_inode+0x18/0x40
[  459.402970]  alloc_inode+0x5f/0x180
[  459.403510]  iget5_locked+0x57/0xd0
[  459.404095]  bdget+0x94/0x4e0
[  459.404607]  bd_acquire+0xfa/0x2c0
[  459.405113]  blkdev_open+0x110/0x290
[  459.405702]  do_dentry_open+0x49e/0x1050
[  459.406340]  path_openat+0x148c/0x3f50
[  459.406926]  do_filp_open+0x1a1/0x280
[  459.407471]  do_sys_open+0x3c3/0x500
[  459.408010]  do_syscall_64+0xc3/0x520
[  459.408572]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[  459.409415]
[  459.409679] Freed by task 1262:
[  459.410212]  __kasan_slab_free+0x129/0x170
[  459.410919]  kmem_cache_free+0xb2/0x2a0
[  459.411564]  rcu_process_callbacks+0xbb2/0x2320
[  459.412318]  __do_softirq+0x225/0x8ac

Fix this by delaying bdput() to the end of blkdev_get() which means we
have finished accessing bdev.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Ming Lei <ming.lei@redhat.com>
Cc: Jan Kara <jack@suse.cz>
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 v2: Add Reported-by tag and cc linux-block mailing list

 fs/block_dev.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 47860e589388..8faec9fb47b6 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1566,7 +1566,6 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 	if (!for_part) {
 		ret = devcgroup_inode_permission(bdev->bd_inode, perm);
 		if (ret != 0) {
-			bdput(bdev);
 			return ret;
 		}
 	}
@@ -1688,7 +1687,6 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 	disk_unblock_events(disk);
 	put_disk_and_module(disk);
  out:
-	bdput(bdev);
 
 	return ret;
 }
@@ -1755,6 +1753,9 @@ int blkdev_get(struct block_device *bdev, fmode_t mode, void *holder)
 		bdput(whole);
 	}
 
+	if (res)
+		bdput(bdev);
+
 	return res;
 }
 EXPORT_SYMBOL(blkdev_get);
-- 
2.21.3