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

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

[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: 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()

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

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

[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: [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: 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()

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

[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: [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()

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

[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