Re: [PATCH v2] block: Fix use-after-free in blkdev_get()
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()
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 SUSE Labs, CR
Re: [PATCH v2] block: Fix use-after-free in blkdev_get()
On Fri, Jun 5, 2020 at 11:46 AM Dan Carpenter 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()
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()
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()
> 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()
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()
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()
> … 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
