diff mbox series

[v2,10/10] block: put_device() if device_add() fails

Message ID 20200419194529.4872-11-mcgrof@kernel.org (mailing list archive)
State New, archived
Headers show
Series block: fix blktrace debugfs use after free | expand

Commit Message

Luis Chamberlain April 19, 2020, 7:45 p.m. UTC
Through code inspection I've found that we don't put_device() if
device_add() fails, and this must be done to decrement its refcount.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
 block/genhd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Bart Van Assche April 19, 2020, 11:40 p.m. UTC | #1
On 4/19/20 12:45 PM, Luis Chamberlain wrote:
> Through code inspection I've found that we don't put_device() if
> device_add() fails, and this must be done to decrement its refcount.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Luis Chamberlain April 24, 2020, 10:32 p.m. UTC | #2
On Sun, Apr 19, 2020 at 04:40:45PM -0700, Bart Van Assche wrote:
> On 4/19/20 12:45 PM, Luis Chamberlain wrote:
> > Through code inspection I've found that we don't put_device() if
> > device_add() fails, and this must be done to decrement its refcount.
> 
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Turns out this is wrong, as bdi needs it still, we have can only remove
it once all users are done, which should be at the disk_release() path.

I've found this while adding the errors paths missing.

  Luis
Bart Van Assche April 25, 2020, 1:58 a.m. UTC | #3
On 2020-04-24 15:32, Luis Chamberlain wrote:
> On Sun, Apr 19, 2020 at 04:40:45PM -0700, Bart Van Assche wrote:
>> On 4/19/20 12:45 PM, Luis Chamberlain wrote:
>>> Through code inspection I've found that we don't put_device() if
>>> device_add() fails, and this must be done to decrement its refcount.
>>
>> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> 
> Turns out this is wrong, as bdi needs it still, we have can only remove
> it once all users are done, which should be at the disk_release() path.
> 
> I've found this while adding the errors paths missing.

Hi Luis,

I had a look at the comments above device_add() before I added my
Reviewed-by. Now that I've had another look at these comments and also
at the device_add() implementation I agree that we don't need this patch.

Bart.
Luis Chamberlain April 25, 2020, 2:12 a.m. UTC | #4
On Fri, Apr 24, 2020 at 06:58:23PM -0700, Bart Van Assche wrote:
> On 2020-04-24 15:32, Luis Chamberlain wrote:
> > On Sun, Apr 19, 2020 at 04:40:45PM -0700, Bart Van Assche wrote:
> >> On 4/19/20 12:45 PM, Luis Chamberlain wrote:
> >>> Through code inspection I've found that we don't put_device() if
> >>> device_add() fails, and this must be done to decrement its refcount.
> >>
> >> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> > 
> > Turns out this is wrong, as bdi needs it still, we have can only remove
> > it once all users are done, which should be at the disk_release() path.
> > 
> > I've found this while adding the errors paths missing.
> 
> Hi Luis,
> 
> I had a look at the comments above device_add() before I added my
> Reviewed-by. Now that I've had another look at these comments and also
> at the device_add() implementation I agree that we don't need this patch.

Thanks for the confirmation. And just to note, we don't do then
put_device() because we don't handle error paths properly. Once we do,
we'll need to ensure we put_disk() just at the right place. I'm working
on putting some final brush strokes on that now.

   Luis
diff mbox series

Patch

diff --git a/block/genhd.c b/block/genhd.c
index 06b642b23a07..c52095a74792 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -721,8 +721,10 @@  static void register_disk(struct device *parent, struct gendisk *disk,
 		WARN_ON(ddev->groups);
 		ddev->groups = groups;
 	}
-	if (device_add(ddev))
+	if (device_add(ddev)) {
+		put_device(ddev);
 		return;
+	}
 	if (!sysfs_deprecated) {
 		err = sysfs_create_link(block_depr, &ddev->kobj,
 					kobject_name(&ddev->kobj));