linux-kernel - Re: [PATCH v4 05/11] block: Add core atomic write support

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 for Android: free password hash cracker in your pocket

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

Message-ID: <f5ccbfac-e369-4371-bb7f-68bb40c47239@oracle.com>
Date: Tue, 20 Feb 2024 10:01:38 +0000
From: John Garry <john.g.garry@...cle.com>
To: Dave Chinner <david@...morbit.com>
Cc: axboe@...nel.dk, kbusch@...nel.org, hch@....de, sagi@...mberg.me,
        jejb@...ux.ibm.com, martin.petersen@...cle.com, djwong@...nel.org,
        viro@...iv.linux.org.uk, brauner@...nel.org, dchinner@...hat.com,
        jack@...e.cz, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-nvme@...ts.infradead.org,
        linux-fsdevel@...r.kernel.org, tytso@....edu, jbongio@...gle.com,
        linux-scsi@...r.kernel.org, ojaswin@...ux.ibm.com, linux-aio@...ck.org,
        linux-btrfs@...r.kernel.org, io-uring@...r.kernel.org,
        nilay@...ux.ibm.com, ritesh.list@...il.com
Subject: Re: [PATCH v4 05/11] block: Add core atomic write support

On 19/02/2024 22:58, Dave Chinner wrote:
> On Mon, Feb 19, 2024 at 01:01:03PM +0000, John Garry wrote:
>> Add atomic write support as follows:
>> diff --git a/block/blk-merge.c b/block/blk-merge.c
>> index 74e9e775f13d..12a75a252ca2 100644
>> --- a/block/blk-merge.c
>> +++ b/block/blk-merge.c
>> @@ -18,6 +18,42 @@
>>   #include "blk-rq-qos.h"
>>   #include "blk-throttle.h"
>>   
>> +static bool rq_straddles_atomic_write_boundary(struct request *rq,
>> +					unsigned int front,
>> +					unsigned int back)
>> +{
>> +	unsigned int boundary = queue_atomic_write_boundary_bytes(rq->q);
>> +	unsigned int mask, imask;
>> +	loff_t start, end;
>> +
>> +	if (!boundary)
>> +		return false;
>> +
>> +	start = rq->__sector << SECTOR_SHIFT;
>> +	end = start + rq->__data_len;
>> +
>> +	start -= front;
>> +	end += back;
>> +
>> +	/* We're longer than the boundary, so must be crossing it */
>> +	if (end - start > boundary)
>> +		return true;
>> +
>> +	mask = boundary - 1;
>> +
>> +	/* start/end are boundary-aligned, so cannot be crossing */
>> +	if (!(start & mask) || !(end & mask))
>> +		return false;
>> +
>> +	imask = ~mask;
>> +
>> +	/* Top bits are different, so crossed a boundary */
>> +	if ((start & imask) != (end & imask))
>> +		return true;
>> +
>> +	return false;
>> +}
> I have no way of verifying this function is doing what it is
> supposed to because it's function is undocumented. I have no idea
> what the front/back variables are supposed to represent, and so no
> clue if they are being applied properly.

I'll add proper function header documentation.

> 
> That said, it's also applying unsigned 32 bit mask variables to
> signed 64 bit quantities and trying to do things like "high bit changed"
> checks on the 64 bit variable. This just smells like a future
> source of "large offsets don't work like we expected!" bugs.

I'll change variables to be unsigned and also all the same size.

> 
>> diff --git a/block/blk-settings.c b/block/blk-settings.c
>> index 06ea91e51b8b..176f26374abc 100644
>> --- a/block/blk-settings.c
>> +++ b/block/blk-settings.c
>> @@ -59,6 +59,13 @@ void blk_set_default_limits(struct queue_limits *lim)
>>   	lim->zoned = false;
>>   	lim->zone_write_granularity = 0;
>>   	lim->dma_alignment = 511;
>> +	lim->atomic_write_hw_max_sectors = 0;
>> +	lim->atomic_write_max_sectors = 0;
>> +	lim->atomic_write_hw_boundary_sectors = 0;
>> +	lim->atomic_write_hw_unit_min_sectors = 0;
>> +	lim->atomic_write_unit_min_sectors = 0;
>> +	lim->atomic_write_hw_unit_max_sectors = 0;
>> +	lim->atomic_write_unit_max_sectors = 0;
>>   }
> Seems to me this function would do better to just
> 
> 	memset(lim, 0, sizeof(*lim));
> 
> and then set all the non-zero fields.

Christoph responded about limits here and in 
blk_queue_atomic_write_max_bytes(), so please let me know if still some 
concerns.

Thanks,
John