Re: [PATCH] fat: fix corruption in fat_alloc_new_dir()

[Jan Stancek <jstancek@redhat.com>]

----- Original Message -----
> Jan Stancek <jstancek@redhat.com> writes:
> 
> > sb_getblk does not guarantee that buffer_head is uptodate. If there is
> > async read running in parallel for same buffer_head, it can overwrite
> > just initialized msdos_dir_entry, leading to corruption:
> >   FAT-fs (loop0): error, corrupted directory (invalid entries)
> >   FAT-fs (loop0): Filesystem has been set read-only
> >
> > This can happen for example during LTP statx04, which creates loop
> > device, formats it (mkfs.vfat), mounts it and immediately creates
> > a new directory. In parallel, systemd-udevd is probing new block
> > device, which leads to async read.
> >
> >   do_mkdirat                      ksys_read
> >    vfs_mkdir                       vfs_read
> >     vfat_mkdir                      __vfs_read
> >      fat_alloc_new_dir               new_sync_read
> >        /* init de[0], de[1] */        blkdev_read_iter
> >                                        generic_file_read_iter
> >                                         generic_file_buffered_read
> >                                          blkdev_readpage
> >                                           block_read_full_page
> >
> > Faster reproducer (based on LTP statx04):
> >
> > int main(void)
> > {
> > 	int i, j, ret, fd, loop_fd, ctrl_fd;
> > 	int loop_num;
> > 	char loopdev[256], tmp[256], testfile[256];
> >
> > 	mkdir("/tmp/mntpoint", 0777);
> > 	for (i = 0; ; i++) {
> > 		printf("Iteration: %d\n", i);
> > 		sprintf(testfile, "/tmp/test.img.%d", getpid());
> >
> > 		ctrl_fd = open("/dev/loop-control", O_RDWR);
> > 		loop_num = ioctl(ctrl_fd, LOOP_CTL_GET_FREE);
> > 		close(ctrl_fd);
> > 		sprintf(loopdev, "/dev/loop%d", loop_num);
> >
> > 		fd = open(testfile, O_WRONLY|O_CREAT|O_TRUNC, 0600);
> > 		fallocate(fd, 0, 0, 256*1024*1024);
> > 		close(fd);
> >
> > 		fd = open(testfile, O_RDWR);
> > 		loop_fd = open(loopdev, O_RDWR);
> > 		ioctl(loop_fd, LOOP_SET_FD, fd);
> > 		close(loop_fd);
> > 		close(fd);
> >
> > 		sprintf(tmp, "mkfs.vfat %s", loopdev);
> > 		system(tmp);
> > 		mount(loopdev, "/tmp/mntpoint", "vfat", 0, NULL);
> >
> > 		for (j = 0; j < 200; j++) {
> > 			sprintf(tmp, "/tmp/mntpoint/testdir%d", j);
> > 			ret = mkdir(tmp, 0777);
> > 			if (ret) {
> > 				perror("mkdir");
> > 				break;
> > 			}
> > 		}
> >
> > 		umount("/tmp/mntpoint");
> > 		loop_fd = open(loopdev, O_RDWR);
> > 		ioctl(loop_fd, LOOP_CLR_FD, fd);
> > 		close(loop_fd);
> > 		unlink(testfile);
> >
> > 		if (ret)
> > 			break;
> > 	}
> >
> > 	return 0;
> > }
> >
> > Issue triggers within minute on HPE Apollo 70 (arm64, 64GB RAM, 224 CPUs).
> 
> Using the device while mounting same device doesn't work reliably like
> this race. (getblk() is intentionally used to get the buffer to write
> new data.)

Are you saying this is expected even if 'usage' is just read?

> 
> mount(2) internally opens the device by EXCL mode, so I guess udev opens
> without EXCL (I dont know if it is intent or not).

I gave this a try and added O_EXCL to udev-builtin-blkid.c. My system had trouble
booting, it was getting stuck on mounting LVM volumes.

So, I'm not sure how to move forward here. 

Regards,
Jan