linux-kernel - Re: [PATCH] inode: For readonly filesystem, func file_update_time should return -EROFS rather than zero.

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: <20131015134604.GF12428@quack.suse.cz>
Date:	Tue, 15 Oct 2013 15:46:04 +0200
From:	Jan Kara <jack@...e.cz>
To:	majianpeng <majianpeng@...il.com>
Cc:	viro <viro@...iv.linux.org.uk>, jbacik <jbacik@...ionio.com>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] inode: For readonly filesystem, func file_update_time
 should return -EROFS rather than zero.

On Tue 15-10-13 11:26:00, majianpeng wrote:
> For ext2 mounted with errors=remount-ro, if write a file and because the
> harddisk error, the fs became ro.But the following test can't return.
> 
> int main()
> {
> 	int ret;
> 	int i = 0;
> 	char buff[SIZE];
> 	int fd = open("/opt/test", O_WRONLY|O_TRUNC|O_CREAT);
> 
> 	if (fd < 0) {
> 		printf("open error %s\n", strerror(errno));
> 		return errno;
> 	}
> 
> 	ret = write(fd, buff, SIZE);
> 	lseek(fd, 0, 0);
> 
> 	while (1) {
> 		ret = write(fd, buff, SIZE);
> 		if (ret < 0) {
> 			printf("write error %s\n", strerror(errno));
> 			break;
> 		}
> 		lseek(fd, 0, 0);
> 		i++;
> 	}
> 	printf("write count=%d\n", i);
> 	close(fd);
> 
> 	return 0;
> }
> 
> For ext3/ext4, because jbd the test can return. But for ext2, because no
> jbd and not reading bitmap from harddisk, the test continue.
> 
> So we should add check readonly-fs on write-path. Func file_update_time
> already check the readonly flag,but it can't return -EROFS.
> For readonly-fs, it can't update a/c/m time of file,it should return
> -EROFS rather than zero.
  The patch looks good to me so feel free to add
Reviewed-by: Jan Kara <jack@...e.cz>
  Just the reasoning in the changelog doesn't look completely correct - we
are free to succeed doing write to a FMODE_WRITE file descriptor even if
the filesystem below it decided to declare game over and switched itself to
read-only mode. It is actually ext2/3/4 which is problematic - its
switching to read-only mode is rather hacky as we cannot really do it
properly from the arbitrary context in which we handle errors. It's not
VFS' fault it doesn't notice the switching...

								Honza

> Signed-off-by: Jianpeng Ma <majianpeng@...il.com>
> ---
>  fs/inode.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index b33ba8e..65302c1 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1644,8 +1644,9 @@ int file_update_time(struct file *file)
>  		return 0;
>  
>  	/* Finally allowed to write? Takes lock. */
> -	if (__mnt_want_write_file(file))
> -		return 0;
> +	ret = __mnt_want_write_file(file);
> +	if (ret)
> +		return ret;
>  
>  	ret = update_time(inode, &now, sync_it);
>  	__mnt_drop_write_file(file);
> -- 
> 1.8.4
-- 
Jan Kara <jack@...e.cz>
SUSE Labs, CR
--
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/