linux-kernel - Re: [PATCH 1/5] block: add flag for add

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 21 Jul 2021 05:59:34 +0100
From:   Christoph Hellwig <hch@...radead.org>
To:     Luis Chamberlain <mcgrof@...nel.org>
Cc:     axboe@...nel.dk, hare@...e.de, bvanassche@....org,
        ming.lei@...hat.com, hch@...radead.org, jack@...e.cz,
        osandov@...com, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] block: add flag for add_disk() completion notation

On Tue, Jul 20, 2021 at 11:20:44AM -0700, Luis Chamberlain wrote:
> Often drivers may have complex setups where it is not
> clear if their disk completed their respective *add_disk*()
> call. They either have to invent a setting or, they
> incorrectly use GENHD_FL_UP. Using GENHD_FL_UP however is
> used internally so we know when we can add / remove
> partitions safely. We can easily fail along the way
> prior to add_disk() completing and still have
> GENHD_FL_UP set, so it would not be correct in that case
> to call del_gendisk() on the disk.
> 
> Provide a new flag then which allows us to check if
> *add_disk*() completed, and conversely just make
> del_gendisk() check for this for drivers so that
> they can safely call del_gendisk() and we'll figure
> it out if it is safe for you to call this.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@...nel.org>
> ---
>  block/genhd.c         |  8 ++++++++
>  include/linux/genhd.h | 11 ++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index af4d2ab4a633..a858eed05e55 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -539,6 +539,8 @@ static void __device_add_disk(struct device *parent, struct gendisk *disk,
>  
>  	disk_add_events(disk);
>  	blk_integrity_add(disk);
> +
> +	disk->flags |= GENHD_FL_DISK_ADDED;

I guess I failed to mention it last time - but I think this needs
to go into disk->state as dynamic state.

> + * Drivers can safely call this even if they are not sure if the respective
> + * __device_add_disk() call succeeded.
> + *
>   * Drivers exist which depend on the release of the gendisk to be synchronous,
>   * it should not be deferred.
>   *
> @@ -578,6 +583,9 @@ void del_gendisk(struct gendisk *disk)
>  {
>  	might_sleep();
>  
> +	if (!blk_disk_added(disk))
> +		return;

I still very much disagree with this check.  It just leads to really
bad driver code.  In genral we need to _fix_ the existing abuses of
the UP check in drivers, not spread this kind of sloppyness further.