Use f_lock to protect f_flags

Traditionally, changes to struct file->f_flags have been done under BKL protection, or with no protection at all. This patch causes all f_flags changes after file open/creation time to be done under protection of f_lock. This allows the removal of some BKL usage and fixes a number of longstanding (if microscopic) races. Reviewed-by: Christoph Hellwig <hch@lst.de> Cc: Al Viro <viro@ZenIV.linux.org.uk> Signed-off-by: Jonathan Corbet <corbet@lwn.net>

Use f_lock to protect f_flags
Traditionally, changes to struct file->f_flags have been done under BKL protection, or with no protection at all. This patch causes all f_flags changes after file open/creation time to be done under protection of f_lock. This allows the removal of some BKL usage and fixes a number of longstanding (if microscopic) races. Reviewed-by: Christoph Hellwig <hch@lst.de> Cc: Al Viro <viro@ZenIV.linux.org.uk> Signed-off-by: Jonathan Corbet <corbet@lwn.net>
Jonathan Corbet
1 parent 6849991490
Showing 13 changed files with 33 additions and 9 deletions Side-by-side Diff
drivers/char/tty_io.c
drivers/usb/gadget/file_storage.c
fs/fcntl.c
fs/ioctl.c
fs/nfsd/vfs.c
include/linux/fs.h
ipc/mqueue.c
sound/core/oss/pcm_oss.c
sound/oss/au1550_ac97.c
sound/oss/audio.c
sound/oss/sh_dac_audio.c
sound/oss/swarm_cs4297a.c
sound/oss/vwsnd.c
@@ -2162,13 +2162,12 @@
 	if (get_user(nonblock, p))
 		return -EFAULT;
  
-	/* file->f_flags is still BKL protected in the fs layer - vomit */
-	lock_kernel();
+	spin_lock(&file->f_lock);
 	if (nonblock)
 		file->f_flags |= O_NONBLOCK;
 	else
 		file->f_flags &= ~O_NONBLOCK;
-	unlock_kernel();
+	spin_unlock(&file->f_lock);
 	return 0;
 }
  
@@ -1711,7 +1711,9 @@
 		curlun->sense_data = SS_WRITE_PROTECTED;
 		return -EINVAL;
 	}
+	spin_lock(&curlun->filp->f_lock);
 	curlun->filp->f_flags &= ~O_SYNC;	// Default is not to wait
+	spin_unlock(&curlun->filp->f_lock);
  
 	/* Get the starting Logical Block Address and check that it's
 	 * not too big */
  
@@ -1728,8 +1730,11 @@
 			curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
 			return -EINVAL;
 		}
-		if (fsg->cmnd[1] & 0x08)	// FUA
+		if (fsg->cmnd[1] & 0x08) {	// FUA
+			spin_lock(&curlun->filp->f_lock);
 			curlun->filp->f_flags |= O_SYNC;
+			spin_unlock(&curlun->filp->f_lock);
+		}
 	}
 	if (lba >= curlun->num_sectors) {
 		curlun->sense_data = SS_LOGICAL_BLOCK_ADDRESS_OUT_OF_RANGE;
@@ -189,7 +189,9 @@
 		}
 	}
  
+	spin_lock(&filp->f_lock);
 	filp->f_flags = (arg & SETFL_MASK) | (filp->f_flags & ~SETFL_MASK);
+	spin_unlock(&filp->f_lock);
  out:
 	unlock_kernel();
 	return error;
@@ -404,10 +404,12 @@
 	if (O_NONBLOCK != O_NDELAY)
 		flag |= O_NDELAY;
 #endif
+	spin_lock(&filp->f_lock);
 	if (on)
 		filp->f_flags |= flag;
 	else
 		filp->f_flags &= ~flag;
+	spin_unlock(&filp->f_lock);
 	return error;
 }
  
  
@@ -432,10 +434,12 @@
 	if (error)
 		return error;
  
+	spin_lock(&filp->f_lock);
 	if (on)
 		filp->f_flags |= FASYNC;
 	else
 		filp->f_flags &= ~FASYNC;
+	spin_unlock(&filp->f_lock);
 	return error;
 }
  
  
@@ -499,10 +503,7 @@
 		break;
  
 	case FIONBIO:
-		/* BKL needed to avoid races tweaking f_flags */
-		lock_kernel();
 		error = ioctl_fionbio(filp, argp);
-		unlock_kernel();
 		break;
  
 	case FIOASYNC:
@@ -998,8 +998,11 @@
  
 	if (!EX_ISSYNC(exp))
 		stable = 0;
-	if (stable && !EX_WGATHER(exp))
+	if (stable && !EX_WGATHER(exp)) {
+		spin_lock(&file->f_lock);
 		file->f_flags |= O_SYNC;
+		spin_unlock(&file->f_lock);
+	}
  
 	/* Write the data. */
 	oldfs = get_fs(); set_fs(KERNEL_DS);
@@ -848,7 +848,7 @@
 #define f_dentry	f_path.dentry
 #define f_vfsmnt	f_path.mnt
 	const struct file_operations	*f_op;
-	spinlock_t		f_lock;  /* f_ep_links */
+	spinlock_t		f_lock;  /* f_ep_links, f_flags */
 	atomic_long_t		f_count;
 	unsigned int 		f_flags;
 	fmode_t			f_mode;
@@ -1156,10 +1156,12 @@
 	omqstat.mq_flags = filp->f_flags & O_NONBLOCK;
 	if (u_mqstat) {
 		audit_mq_getsetattr(mqdes, &mqstat);
+		spin_lock(&filp->f_lock);
 		if (mqstat.mq_flags & O_NONBLOCK)
 			filp->f_flags |= O_NONBLOCK;
 		else
 			filp->f_flags &= ~O_NONBLOCK;
+		spin_unlock(&filp->f_lock);
  
 		inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	}
@@ -1895,7 +1895,9 @@
  
 static int snd_pcm_oss_nonblock(struct file * file)
 {
+	spin_lock(&file->f_lock);
 	file->f_flags |= O_NONBLOCK;
+	spin_unlock(&file->f_lock);
 	return 0;
 }
  
@@ -1627,7 +1627,9 @@
 				    sizeof(abinfo)) ? -EFAULT : 0;
  
 	case SNDCTL_DSP_NONBLOCK:
+		spin_lock(&file->f_lock);
 		file->f_flags |= O_NONBLOCK;
+		spin_unlock(&file->f_lock);
 		return 0;
  
 	case SNDCTL_DSP_GETODELAY:
@@ -433,7 +433,9 @@
 			return dma_ioctl(dev, cmd, arg);
  
 		case SNDCTL_DSP_NONBLOCK:
+			spin_lock(&file->f_lock);
 			file->f_flags |= O_NONBLOCK;
+			spin_unlock(&file->f_lock);
 			return 0;
  
 		case SNDCTL_DSP_GETCAPS:
@@ -135,7 +135,9 @@
 		return put_user(AFMT_U8, (int *)arg);
  
 	case SNDCTL_DSP_NONBLOCK:
+		spin_lock(&file->f_lock);
 		file->f_flags |= O_NONBLOCK;
+		spin_unlock(&file->f_lock);
 		return 0;
  
 	case SNDCTL_DSP_GETCAPS:
@@ -2200,7 +2200,9 @@
 				    sizeof(abinfo)) ? -EFAULT : 0;
  
 	case SNDCTL_DSP_NONBLOCK:
+		spin_lock(&file->f_lock);
 		file->f_flags |= O_NONBLOCK;
+		spin_unlock(&file->f_lock);
 		return 0;
  
 	case SNDCTL_DSP_GETODELAY:
@@ -2673,7 +2673,9 @@
  
 	case SNDCTL_DSP_NONBLOCK:	/* _SIO  ('P',14) */
 		DBGX("SNDCTL_DSP_NONBLOCK\n");
+		spin_lock(&file->f_lock);
 		file->f_flags |= O_NONBLOCK;
+		spin_unlock(&file->f_lock);
 		return 0;
  
 	case SNDCTL_DSP_RESET:		/* _SIO  ('P', 0) */
...	...	@@ -2162,13 +2162,12 @@
2162	2162	if (get_user(nonblock, p))
2163	2163	return -EFAULT;
2164	2164
2165		- /* file->f_flags is still BKL protected in the fs layer - vomit */
2166		- lock_kernel();
	2165	+ spin_lock(&file->f_lock);
2167	2166	if (nonblock)
2168	2167	file->f_flags \|= O_NONBLOCK;
2169	2168	else
2170	2169	file->f_flags &= ~O_NONBLOCK;
2171		- unlock_kernel();
	2170	+ spin_unlock(&file->f_lock);
2172	2171	return 0;
2173	2172	}
2174	2173
...	...	@@ -1711,7 +1711,9 @@
1711	1711	curlun->sense_data = SS_WRITE_PROTECTED;
1712	1712	return -EINVAL;
1713	1713	}
	1714	+ spin_lock(&curlun->filp->f_lock);
1714	1715	curlun->filp->f_flags &= ~O_SYNC; // Default is not to wait
	1716	+ spin_unlock(&curlun->filp->f_lock);
1715	1717
1716	1718	/* Get the starting Logical Block Address and check that it's
1717	1719	* not too big */
1718	1720
...	...	@@ -1728,8 +1730,11 @@
1728	1730	curlun->sense_data = SS_INVALID_FIELD_IN_CDB;
1729	1731	return -EINVAL;
1730	1732	}
1731		- if (fsg->cmnd[1] & 0x08) // FUA
	1733	+ if (fsg->cmnd[1] & 0x08) { // FUA
	1734	+ spin_lock(&curlun->filp->f_lock);
1732	1735	curlun->filp->f_flags \|= O_SYNC;
	1736	+ spin_unlock(&curlun->filp->f_lock);
	1737	+ }
1733	1738	}
1734	1739	if (lba >= curlun->num_sectors) {
1735	1740	curlun->sense_data = SS_LOGICAL_BLOCK_ADDRESS_OUT_OF_RANGE;
...	...	@@ -189,7 +189,9 @@
189	189	}
190	190	}
191	191
	192	+ spin_lock(&filp->f_lock);
192	193	filp->f_flags = (arg & SETFL_MASK) \| (filp->f_flags & ~SETFL_MASK);
	194	+ spin_unlock(&filp->f_lock);
193	195	out:
194	196	unlock_kernel();
195	197	return error;
...	...	@@ -404,10 +404,12 @@
404	404	if (O_NONBLOCK != O_NDELAY)
405	405	flag \|= O_NDELAY;
406	406	#endif
	407	+ spin_lock(&filp->f_lock);
407	408	if (on)
408	409	filp->f_flags \|= flag;
409	410	else
410	411	filp->f_flags &= ~flag;
	412	+ spin_unlock(&filp->f_lock);
411	413	return error;
412	414	}
413	415
414	416
...	...	@@ -432,10 +434,12 @@
432	434	if (error)
433	435	return error;
434	436
	437	+ spin_lock(&filp->f_lock);
435	438	if (on)
436	439	filp->f_flags \|= FASYNC;
437	440	else
438	441	filp->f_flags &= ~FASYNC;
	442	+ spin_unlock(&filp->f_lock);
439	443	return error;
440	444	}
441	445
442	446
...	...	@@ -499,10 +503,7 @@
499	503	break;
500	504
501	505	case FIONBIO:
502		- /* BKL needed to avoid races tweaking f_flags */
503		- lock_kernel();
504	506	error = ioctl_fionbio(filp, argp);
505		- unlock_kernel();
506	507	break;
507	508
508	509	case FIOASYNC:
...	...	@@ -998,8 +998,11 @@
998	998
999	999	if (!EX_ISSYNC(exp))
1000	1000	stable = 0;
1001		- if (stable && !EX_WGATHER(exp))
	1001	+ if (stable && !EX_WGATHER(exp)) {
	1002	+ spin_lock(&file->f_lock);
1002	1003	file->f_flags \|= O_SYNC;
	1004	+ spin_unlock(&file->f_lock);
	1005	+ }
1003	1006
1004	1007	/* Write the data. */
1005	1008	oldfs = get_fs(); set_fs(KERNEL_DS);
...	...	@@ -848,7 +848,7 @@
848	848	#define f_dentry f_path.dentry
849	849	#define f_vfsmnt f_path.mnt
850	850	const struct file_operations *f_op;
851		- spinlock_t f_lock; /* f_ep_links */
	851	+ spinlock_t f_lock; /* f_ep_links, f_flags */
852	852	atomic_long_t f_count;
853	853	unsigned int f_flags;
854	854	fmode_t f_mode;
...	...	@@ -1156,10 +1156,12 @@
1156	1156	omqstat.mq_flags = filp->f_flags & O_NONBLOCK;
1157	1157	if (u_mqstat) {
1158	1158	audit_mq_getsetattr(mqdes, &mqstat);
	1159	+ spin_lock(&filp->f_lock);
1159	1160	if (mqstat.mq_flags & O_NONBLOCK)
1160	1161	filp->f_flags \|= O_NONBLOCK;
1161	1162	else
1162	1163	filp->f_flags &= ~O_NONBLOCK;
	1164	+ spin_unlock(&filp->f_lock);
1163	1165
1164	1166	inode->i_atime = inode->i_ctime = CURRENT_TIME;
1165	1167	}
...	...	@@ -1895,7 +1895,9 @@
1895	1895
1896	1896	static int snd_pcm_oss_nonblock(struct file * file)
1897	1897	{
	1898	+ spin_lock(&file->f_lock);
1898	1899	file->f_flags \|= O_NONBLOCK;
	1900	+ spin_unlock(&file->f_lock);
1899	1901	return 0;
1900	1902	}
1901	1903
...	...	@@ -1627,7 +1627,9 @@
1627	1627	sizeof(abinfo)) ? -EFAULT : 0;
1628	1628
1629	1629	case SNDCTL_DSP_NONBLOCK:
	1630	+ spin_lock(&file->f_lock);
1630	1631	file->f_flags \|= O_NONBLOCK;
	1632	+ spin_unlock(&file->f_lock);
1631	1633	return 0;
1632	1634
1633	1635	case SNDCTL_DSP_GETODELAY:
...	...	@@ -433,7 +433,9 @@
433	433	return dma_ioctl(dev, cmd, arg);
434	434
435	435	case SNDCTL_DSP_NONBLOCK:
	436	+ spin_lock(&file->f_lock);
436	437	file->f_flags \|= O_NONBLOCK;
	438	+ spin_unlock(&file->f_lock);
437	439	return 0;
438	440
439	441	case SNDCTL_DSP_GETCAPS: