lkml.org 
[lkml]   [2018]   [Sep]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4] block/loop: Serialize ioctl operations.
From
Date
On 2018/09/25 3:47, Jan Kara wrote:
>> +/*
>> + * unlock_loop - Unlock loop_mutex as needed.
>> + *
>> + * Explicitly call this function before calling fput() or blkdev_reread_part()
>> + * in order to avoid circular lock dependency. After this function is called,
>> + * current thread is no longer allowed to access "struct loop_device" memory,
>> + * for another thread would access that memory as soon as loop_mutex is held.
>> + */
>> +static void unlock_loop(void)
>> +{
>> + if (loop_mutex_owner == current) {
>
> Urgh, why this check? Conditional locking / unlocking is evil so it has to
> have *very* good reasons and there is not any explanation here. So far I
> don't see a reason why this is needed at all.

Yeah, this is why Jens hates this patch. But any alternative?



>> @@ -630,7 +669,12 @@ static void loop_reread_partitions(struct loop_device *lo,
>> + unlock_loop();
>
> Unlocking in loop_reread_partitions() makes the locking rules ugly. And
> unnecessarily AFAICT. Can't we just use lo_refcnt to protect us against
> loop_clr_fd() and freeing of 'lo' structure itself?

Really? I think that just elevating lo->lo_refcnt will cause another lockdep
warning because __blkdev_reread_part() requires bdev->bd_mutex being held.
Don't we need to drop the lock in order to solve original lockdep warning at [2] ?

int __blkdev_reread_part(struct block_device *bdev)
{
struct gendisk *disk = bdev->bd_disk;

if (!disk_part_scan_enabled(disk) || bdev != bdev->bd_contains)
return -EINVAL;
if (!capable(CAP_SYS_ADMIN))
return -EACCES;

lockdep_assert_held(&bdev->bd_mutex);

return rescan_partitions(disk, bdev);
}

int blkdev_reread_part(struct block_device *bdev)
{
int res;

mutex_lock(&bdev->bd_mutex);
res = __blkdev_reread_part(bdev);
mutex_unlock(&bdev->bd_mutex);

return res;
}

\
 
 \ /
  Last update: 2018-09-24 23:07    [W:2.057 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site