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