diff mbox

AW: Locking problems with Linux 4.9 and 4.11 with NFSD and `fs/iomap.c`

Message ID 20170811101513.GA11531@lst.de (mailing list archive)
State New, archived
Headers show

Commit Message

Christoph Hellwig Aug. 11, 2017, 10:15 a.m. UTC
On Thu, Aug 10, 2017 at 07:54:51PM +0000, Markus Stockhausen wrote:
> Lets say you are trying to zero multiple of 4GB chunks. With bytes
> evaluated towards 0 this will hit an endless loop within that iomap
> function. That might explain your observation. If that is right a bugfix
> would qualify for stable 4.8+

Yes, it seems like min_t casts arguments 2 and 3 to the type in argument
1, which could lead to incorrect truncation.

Paul, please try the patch below:

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Paul Menzel Aug. 11, 2017, 3:14 p.m. UTC | #1
Dear Christoph, dear Markus,


On 08/11/17 12:15, Christoph Hellwig wrote:
> On Thu, Aug 10, 2017 at 07:54:51PM +0000, Markus Stockhausen wrote:
>> Lets say you are trying to zero multiple of 4GB chunks. With bytes
>> evaluated towards 0 this will hit an endless loop within that iomap
>> function. That might explain your observation. If that is right a bugfix
>> would qualify for stable 4.8+
> 
> Yes, it seems like min_t casts arguments 2 and 3 to the type in argument
> 1, which could lead to incorrect truncation.
> 
> Paul, please try the patch below:
> 
> diff --git a/fs/iomap.c b/fs/iomap.c
> index 039266128b7f..59cc98ad7577 100644
> --- a/fs/iomap.c
> +++ b/fs/iomap.c
> @@ -278,7 +278,7 @@ iomap_dirty_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
>   		unsigned long bytes;	/* Bytes to write to page */
>   
>   		offset = (pos & (PAGE_SIZE - 1));
> -		bytes = min_t(unsigned long, PAGE_SIZE - offset, length);
> +		bytes = min_t(loff_t, PAGE_SIZE - offset, length);
>   
>   		rpage = __iomap_read_page(inode, pos);
>   		if (IS_ERR(rpage))
> @@ -373,7 +373,7 @@ iomap_zero_range_actor(struct inode *inode, loff_t pos, loff_t count,
>   		unsigned offset, bytes;
>   
>   		offset = pos & (PAGE_SIZE - 1); /* Within page */
> -		bytes = min_t(unsigned, PAGE_SIZE - offset, count);
> +		bytes = min_t(loff_t, PAGE_SIZE - offset, count);
>   
>   		if (IS_DAX(inode))
>   			status = iomap_dax_zero(pos, offset, bytes, iomap);

I applied this on top of Linux 4.9.41. Even when writing 40 100 GB in 
parallel on an NFS exported directory from different systems, and 
keeping the load high on the system, the NFS exported directory was 
always accessible from all hosts.

Markus, thank you very much for looking into this and spotting the bug. 
A bug thank you to all people helping to analyze this issue.

I’ll keep you posted, but until know.

Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/fs/iomap.c b/fs/iomap.c
index 039266128b7f..59cc98ad7577 100644
--- a/fs/iomap.c
+++ b/fs/iomap.c
@@ -278,7 +278,7 @@  iomap_dirty_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
 		unsigned long bytes;	/* Bytes to write to page */
 
 		offset = (pos & (PAGE_SIZE - 1));
-		bytes = min_t(unsigned long, PAGE_SIZE - offset, length);
+		bytes = min_t(loff_t, PAGE_SIZE - offset, length);
 
 		rpage = __iomap_read_page(inode, pos);
 		if (IS_ERR(rpage))
@@ -373,7 +373,7 @@  iomap_zero_range_actor(struct inode *inode, loff_t pos, loff_t count,
 		unsigned offset, bytes;
 
 		offset = pos & (PAGE_SIZE - 1); /* Within page */
-		bytes = min_t(unsigned, PAGE_SIZE - offset, count);
+		bytes = min_t(loff_t, PAGE_SIZE - offset, count);
 
 		if (IS_DAX(inode))
 			status = iomap_dax_zero(pos, offset, bytes, iomap);