linux-kernel - Re: [PATCH 22/31] fs/ntfs: use kmemdup rather than duplicating its implementation

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

Date:	Thu, 24 Sep 2015 10:20:30 +0100
From:	Anton Altaparmakov <anton@...era.com>
To:	Andrzej Hajda <a.hajda@...sung.com>
Cc:	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-ntfs-dev@...ts.sourceforge.net" 
	<linux-ntfs-dev@...ts.sourceforge.net>
Subject: Re: [PATCH 22/31] fs/ntfs: use kmemdup rather than duplicating its implementation

Hi Andrzej,

> On 24 Sep 2015, at 09:34, Andrzej Hajda <a.hajda@...sung.com> wrote:
> 
> On 09/23/2015 12:21 PM, Anton Altaparmakov wrote:
>> Hi Andrzej,
>> 
>> Thanks for your patch.  It looks fine though I don't quite see the point of it to be honest.
>> 
>> It actually adds an additional function call (kmemdup() is not inline) just to save 1 line of source code in the driver and I don't think it improves readability or anything so why bother?  What does it gain?
> 
> kmemdup replaces combo (kmalloc + memdup) with one call.
> The patch follows quite common practice of abstracting out common patterns.

Sure it does I am just questioning the sanity of the practice...  (-;

Such changes reduce the size of the kernel binary by a few bytes at the cost of adding CPU cycles to the execution time.  How is that good thing?  Unless you are on an embedded system desperate for RAM throwing away CPU cycles on pointless abstractions makes no sense to me...

But as I said patch is fine.  Feel free to send it onto Andrew to get it into mainline.  You can add my Acked-by: Anton Altaparmakoc <anton@...era.com> line to it when sending it.  I am just saying that I think patches like that don't make much sense to me...

Best regards,

	Anton

> Regards
> Andrzej
> 
>> 
>> Best regards,
>> 
>> 	Anton
>> 
>>> On 7 Aug 2015, at 08:59, Andrzej Hajda <a.hajda@...sung.com> wrote:
>>> 
>>> The patch was generated using fixed coccinelle semantic patch
>>> scripts/coccinelle/api/memdup.cocci [1].
>>> 
>>> [1]: http://permalink.gmane.org/gmane.linux.kernel/2014320
>>> 
>>> Signed-off-by: Andrzej Hajda <a.hajda@...sung.com>
>>> ---
>>> fs/ntfs/dir.c | 7 +++----
>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/fs/ntfs/dir.c b/fs/ntfs/dir.c
>>> index 9e38daf..2b7fef0 100644
>>> --- a/fs/ntfs/dir.c
>>> +++ b/fs/ntfs/dir.c
>>> @@ -1172,14 +1172,13 @@ static int ntfs_readdir(struct file *file, struct dir_context *actor)
>>> 	 * map the mft record without deadlocking.
>>> 	 */
>>> 	rc = le32_to_cpu(ctx->attr->data.resident.value_length);
>>> -	ir = kmalloc(rc, GFP_NOFS);
>>> +	/* Copy the index root value (it has been verified in read_inode). */
>>> +	ir = kmemdup((u8 *)ctx->attr + le16_to_cpu(ctx->attr->data.resident.value_offset),
>>> +		     rc, GFP_NOFS);
>>> 	if (unlikely(!ir)) {
>>> 		err = -ENOMEM;
>>> 		goto err_out;
>>> 	}
>>> -	/* Copy the index root value (it has been verified in read_inode). */
>>> -	memcpy(ir, (u8*)ctx->attr +
>>> -			le16_to_cpu(ctx->attr->data.resident.value_offset), rc);
>>> 	ntfs_attr_put_search_ctx(ctx);
>>> 	unmap_mft_record(ndir);
>>> 	ctx = NULL;
>>> -- 
>>> 1.9.1

-- 
Anton Altaparmakov <anton at tuxera.com> (replace at with @)
Lead in File System Development, Tuxera Inc., http://www.tuxera.com/
Linux NTFS maintainer

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/