linux-kernel - Re: [PATCH v2 4/4] scatterlist: add sgl

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] [day] [month] [year] [list]

Message-ID: <75d9b1cf-e418-cee1-89de-c59c5b2b4304@gmail.com>
Date:   Mon, 19 Oct 2020 13:18:37 +0200
From:   Bodo Stroesser <bostroesser@...il.com>
To:     Douglas Gilbert <dgilbert@...erlog.com>,
        linux-scsi@...r.kernel.org, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org
Cc:     martin.petersen@...cle.com, axboe@...nel.dk, bvanassche@....org
Subject: Re: [PATCH v2 4/4] scatterlist: add sgl_memset()

AFAICS, there are 2 unneeded lines in the new implementation
of sgl_memset. Please see details below.


Am 18.10.20 um 19:13 schrieb Douglas Gilbert:
> The existing sg_zero_buffer() function is a bit restrictive.
> For example protection information (PI) blocks are usually
> initialized to 0xff bytes. As its name suggests sgl_memset()
> is modelled on memset(). One difference is the type of the
> val argument which is u8 rather than int. Plus it returns
> the number of bytes (over)written.
> 
> Signed-off-by: Douglas Gilbert <dgilbert@...erlog.com>
> ---

...

> +
> +/**
> + * sgl_memset - set byte 'val' up to n_bytes times on SG list
> + * @sgl:		 The SG list
> + * @nents:		 Number of SG entries in sgl
> + * @skip:		 Number of bytes to skip before starting
> + * @val:		 byte value to write to sgl
> + * @n_bytes:		 The (maximum) number of bytes to modify
> + *
> + * Returns:
> + *   The number of bytes written.
> + *
> + * Notes:
> + *   Stops writing if either sgl or n_bytes is exhausted. If n_bytes is
> + *   set SIZE_MAX then val will be written to each byte until the end
> + *   of sgl.
> + *
> + *   The notes in sgl_copy_sgl() about large sgl_s _applies here as well.
> + *
> + **/
> +size_t sgl_memset(struct scatterlist *sgl, unsigned int nents, off_t skip,
> +		  u8 val, size_t n_bytes)
> +{
> +	size_t offset = 0;
> +	size_t len;
> +	struct sg_mapping_iter miter;
> +
> +	if (n_bytes == 0)
> +		return 0;
> +	sg_miter_start(&miter, sgl, nents, SG_MITER_ATOMIC | SG_MITER_TO_SG);
> +	if (!sg_miter_skip(&miter, skip))
> +		goto fini;
> +
> +	while ((offset < n_bytes) && sg_miter_next(&miter)) {
> +		len = min(miter.length, n_bytes - offset);
> +		memset(miter.addr, val, len);
> +		offset += len;
> +		miter.consumed = len;

The above line will not change miter.consumed in all loop cycles but the
last, since len will be miter.length for all loop cycles but the last
and sg_miter_next initializes miter.consumed to contain miter.length.
In the last loop cycle it does not harm if miter.consumed stays bigger
than len. So this line is not needed and can be removed.

> +		sg_miter_stop(&miter);

Since the code does not use nested sg_miter, the sg_miter_stop() here is
not needed, you can remove that line.

Either the next call to sg_miter_next will call sg_miter_stop before
preparing next chunk of mem, or sg_miter_stop is called behind the loop.

> +	}
> +fini:
> +	sg_miter_stop(&miter);
> +	return offset;
> +}
> +EXPORT_SYMBOL(sgl_memset);
> +
>