linux-kernel - Re: [PATCH 2/2] loop: support 4k physical blocksize

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:   Thu, 3 Nov 2016 15:26:44 +0100
From:   Christoph Hellwig <hch@....de>
To:     Hannes Reinecke <hare@...e.de>
Cc:     Jens Axboe <axboe@...com>, Christoph Hellwig <hch@....de>,
        Alexander Graf <agraf@...e.com>,
        Ming Lei <tom.leiming@...il.com>,
        Martin Schwidefsky <schwidefsky@...ibm.com>,
        linux-kernel@...r.kernel.org, Hannes Reinecke <hare@...e.com>
Subject: Re: [PATCH 2/2] loop: support 4k physical blocksize

> -figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit)
> +figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit,
> +		 loff_t logical_blocksize)
>  {
>  	loff_t size = get_size(offset, sizelimit, lo->lo_backing_file);
>  	sector_t x = (sector_t)size;
> @@ -233,6 +234,12 @@ figure_loop_size(struct loop_device *lo, loff_t offset, loff_t sizelimit)
>  		lo->lo_offset = offset;
>  	if (lo->lo_sizelimit != sizelimit)
>  		lo->lo_sizelimit = sizelimit;
> +	if (lo->lo_flags & LO_FLAGS_BLOCKSIZE) {
> +		lo->lo_logical_blocksize = logical_blocksize;
> +		blk_queue_physical_block_size(lo->lo_queue, lo->lo_blocksize);
> +		blk_queue_logical_block_size(lo->lo_queue,
> +					     lo->lo_logical_blocksize);
> +	}

I've looked how the existing code uses lo_blocksize and this whole thing
confuses me to no end.  Can we have all the blocksize assignments (i.e.
including the existing lo_blocksize assignments) in one single place and
documented?  Especialy as it seems so far lo_blocksize seems to be
the "fs" blocksize as set by set_blocksize, which seems totally wrong
to be set by loop at all.

> +	if (info->lo_flags & LO_FLAGS_BLOCKSIZE) {
> +		lo->lo_flags |= LO_FLAGS_BLOCKSIZE;
> +		if ((info->lo_init[0] != 512) &&
> +		    (info->lo_init[0] != 1024) &&
> +		    (info->lo_init[0] != 2048) &&
> +		    (info->lo_init[0] != 4096))
> +			return -EINVAL;

No need for the inner braces.  Also please find a way to have a
descriptive name for the field, be that an anonymous union, or a #define.

> +	     (lo->lo_logical_blocksize != info->lo_init[0])))

Same comment about the inner braces here.

> +		if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit,
> +				     info->lo_init[0]))
>  			return -EFBIG;
>  
>  	loop_config_discard(lo);
> @@ -1303,7 +1328,8 @@ static int loop_set_capacity(struct loop_device *lo)
>  	if (unlikely(lo->lo_state != Lo_bound))
>  		return -ENXIO;
>  
> -	return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit);
> +	return figure_loop_size(lo, lo->lo_offset, lo->lo_sizelimit,
> +				lo->lo_logical_blocksize);

I'd say drop all the arguments but lo here (maybe in a prep patch) as
passing them all seems pointless and confusing.