linux-kernel - Re: [PATCH 1/1] fs/ntfs/mft.c: fix 2 sparse warnings

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

Message-Id: <6E770B36-E0D6-4B98-B1AB-DE92E456BD72@tuxera.com>
Date:	Mon, 28 Apr 2014 16:13:06 +0100
From:	Anton Altaparmakov <anton@...era.com>
To:	Fabian Frederick <fabf@...net.be>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	akpm <akpm@...ux-foundation.org>
Subject: Re: [PATCH 1/1] fs/ntfs/mft.c: fix 2 sparse warnings

NAK.

I do not see why this is needed.  Not all sparse warnings need fixing.

What is so bad about a variable length array that we need to incur kmalloc overhead on each call?  I would have thought the CPU overhead of the kmalloc is a lot larger than any CPU overhead of a variable length array...

The array size is tiny - basically on all volumes mft_record_size is 1024 and blocksize is either 512 or 1024 thus the array size is either 1 or 2 pointers.  The reason it is dynamic is just in case Microsoft change their mind and increase the mft_record_size in a future NTFS/Windows version to something bigger than 1024 bytes, the current code would still cope fine.

Note mft_record_size is the "on disk inode size" thus it will never be much bigger and unlikely to ever change from the 1024 really.  A very long time ago when NTFS was being developed Microsoft used 2048 bytes but quickly changed it to 1024 as most inodes do not need anywhere near that much space for an inode.

Best regards,

	Anton

On 27 Apr 2014, at 11:12, Fabian Frederick <fabf@...net.be> wrote:

> fs/ntfs/mft.c:471:33: warning: Variable length array is used.
> fs/ntfs/mft.c:676:33: warning: Variable length array is used.
> 
> This is untested.
> 
> Cc: Anton Altaparmakov <anton@...era.com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Signed-off-by: Fabian Frederick <fabf@...net.be>
> ---
> fs/ntfs/mft.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
> index 3014a36..eddb739 100644
> --- a/fs/ntfs/mft.c
> +++ b/fs/ntfs/mft.c
> @@ -468,7 +468,7 @@ int ntfs_sync_mft_mirror(ntfs_volume *vol, const unsigned long mft_no,
> 	struct page *page;
> 	unsigned int blocksize = vol->sb->s_blocksize;
> 	int max_bhs = vol->mft_record_size / blocksize;
> -	struct buffer_head *bhs[max_bhs];
> +	struct buffer_head **bhs;
> 	struct buffer_head *bh, *head;
> 	u8 *kmirr;
> 	runlist_element *rl;
> @@ -478,11 +478,14 @@ int ntfs_sync_mft_mirror(ntfs_volume *vol, const unsigned long mft_no,
> 
> 	ntfs_debug("Entering for inode 0x%lx.", mft_no);
> 	BUG_ON(!max_bhs);
> -	if (unlikely(!vol->mftmirr_ino)) {
> +	bhs = kmalloc(max_bhs * sizeof(struct buffer_head *), GFP_NOFS);
> +	if (unlikely(!bhs || !vol->mftmirr_ino)) {
> 		/* This could happen during umount... */
> 		err = ntfs_sync_mft_mirror_umount(vol, mft_no, m);
> -		if (likely(!err))
> +		if (likely(!err)) {
> +			kfree(bhs);
> 			return err;
> +		}
> 		goto err_out;
> 	}
> 	/* Get the page containing the mirror copy of the mft record @m. */
> @@ -632,6 +635,7 @@ err_out:
> 				"after umounting to correct this.", -err);
> 		NVolSetErrors(vol);
> 	}
> +	kfree(bhs);
> 	return err;
> }
> 
> @@ -673,7 +677,7 @@ int write_mft_record_nolock(ntfs_inode *ni, MFT_RECORD *m, int sync)
> 	unsigned int blocksize = vol->sb->s_blocksize;
> 	unsigned char blocksize_bits = vol->sb->s_blocksize_bits;
> 	int max_bhs = vol->mft_record_size / blocksize;
> -	struct buffer_head *bhs[max_bhs];
> +	struct buffer_head **bhs;
> 	struct buffer_head *bh, *head;
> 	runlist_element *rl;
> 	unsigned int block_start, block_end, m_start, m_end;
> @@ -689,7 +693,8 @@ int write_mft_record_nolock(ntfs_inode *ni, MFT_RECORD *m, int sync)
> 	 * There is no danger of races since the caller is holding the locks
> 	 * for the mft record @m and the page it is in.
> 	 */
> -	if (!NInoTestClearDirty(ni))
> +	bhs = kmalloc(max_bhs * sizeof(struct buffer_head *), GFP_NOFS);
> +	if (!bhs || !NInoTestClearDirty(ni))
> 		goto done;
> 	bh = head = page_buffers(page);
> 	BUG_ON(!bh);
> @@ -820,6 +825,7 @@ int write_mft_record_nolock(ntfs_inode *ni, MFT_RECORD *m, int sync)
> 		goto err_out;
> 	}
> done:
> +	kfree(bhs);
> 	ntfs_debug("Done.");
> 	return 0;
> cleanup_out:
> @@ -840,6 +846,7 @@ err_out:
> 		err = 0;
> 	} else
> 		NVolSetErrors(vol);
> +	kfree(bhs);
> 	return err;
> }
--
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/