linux-kernel - Re: PROBLEM: 2.4.36.1 hangs.

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

Date:	Tue, 26 Feb 2008 09:16:25 +0100
From:	Willy Tarreau <w@....eu>
To:	Glen Nakamura <glen@...dulo.com>
Cc:	linux-kernel@...r.kernel.org, dann frazier <dannf@...ian.org>
Subject: Re: PROBLEM: 2.4.36.1 hangs.

On Mon, Feb 25, 2008 at 09:36:12PM -1000, Glen Nakamura wrote:
> Aloha,
> 
> The "ext2_readdir() filp->f_pos fix" patch looks weird...
> Perhaps the "filp->f_pos += le16_to_cpu(de->rec_len);" line should be
> outside of the if statement like the indentation implies?

good catch! At least it's what is done in 2.6.

> As it is, filp->f_pos gets corrupted if de->inode is ever zero...
> This could possibly explain why I had a few strange directory
> entries until I checked the filesystem with:
> e2fsck -D -F -f /dev/{ext2 partition}
> 
> - glen
> 
> Here is an updated (untested) patch:

unfortunately, neither Dann nor me could reproduce the issue, so
we'll wait for some victims^Wvolunteers to give it a try.

BTW, I notice that 2.6 also has one extra chunk that 2.4 does not
have :

                if (unlikely(need_revalidate)) {
+                       if (offset) {
                                offset = ext2_validate_entry(kaddr, offset, chunk_mask);
+                               filp->f_pos = (n<<PAGE_CACHE_SHIFT) + offset;
+                       }
+                       filp->f_version = inode->i_version;
                        need_revalidate = 0;
                }

I have no idea whether this part is needed, we'd better ask Theo or Al
for some advices, as I'm not tempted by merging an uncertain patch when
it comes to filesystems.

Regards,
Willy

> --- linux-2.4.36.orig/fs/ext2/dir.c
> +++ linux-2.4.36/fs/ext2/dir.c
> @@ -240,7 +240,7 @@ ext2_readdir (struct file * filp, void *
>  	loff_t pos = filp->f_pos;
>  	struct inode *inode = filp->f_dentry->d_inode;
>  	struct super_block *sb = inode->i_sb;
> -	unsigned offset = pos & ~PAGE_CACHE_MASK;
> +	unsigned int offset = pos & ~PAGE_CACHE_MASK;
>  	unsigned long n = pos >> PAGE_CACHE_SHIFT;
>  	unsigned long npages = dir_pages(inode);
>  	unsigned chunk_mask = ~(ext2_chunk_size(inode)-1);
> @@ -258,8 +258,13 @@ ext2_readdir (struct file * filp, void *
>  		ext2_dirent *de;
>  		struct page *page = ext2_get_page(inode, n);
>  
> -		if (IS_ERR(page))
> +		if (IS_ERR(page)) {
> +			ext2_error(sb, __FUNCTION__,
> +				   "bad page in #%lu",
> +				   inode->i_ino);
> +			filp->f_pos += PAGE_CACHE_SIZE - offset;
>  			continue;
> +		}
>  		kaddr = page_address(page);
>  		if (need_revalidate) {
>  			offset = ext2_validate_entry(kaddr, offset, chunk_mask);
> @@ -267,7 +272,7 @@ ext2_readdir (struct file * filp, void *
>  		}
>  		de = (ext2_dirent *)(kaddr+offset);
>  		limit = kaddr + PAGE_CACHE_SIZE - EXT2_DIR_REC_LEN(1);
> -		for ( ;(char*)de <= limit; de = ext2_next_entry(de))
> +		for ( ;(char*)de <= limit; de = ext2_next_entry(de)) {
>  			if (de->inode) {
>  				int over;
>  				unsigned char d_type = DT_UNKNOWN;
> @@ -284,11 +289,12 @@ ext2_readdir (struct file * filp, void *
>  					goto done;
>  				}
>  			}
> +			filp->f_pos += le16_to_cpu(de->rec_len);
> +		}
>  		ext2_put_page(page);
>  	}
>  
>  done:
> -	filp->f_pos = (n << PAGE_CACHE_SHIFT) | offset;
>  	filp->f_version = inode->i_version;
>  	UPDATE_ATIME(inode);
>  	return 0;
--
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/