deal with races between remove_proc_entry() and proc_reg_release()

* serialize the call of ->release() on per-pdeo mutex * don't remove pdeo from per-pde list until we are through with it Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

deal with races between remove_proc_entry() and proc_reg_release()
* serialize the call of ->release() on per-pdeo mutex * don't remove pdeo from per-pde list until we are through with it Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Al Viro
1 parent 866ad9a747
Showing 2 changed files with 34 additions and 53 deletions Side-by-side Diff
fs/proc/inode.c
fs/proc/internal.h
@@ -156,6 +156,29 @@
 	spin_unlock(&pde->pde_unload_lock);
 }
  
+/* pde is locked */
+static void close_pdeo(struct proc_dir_entry *pde, struct pde_opener *pdeo)
+{
+	pdeo->count++;
+	if (!mutex_trylock(&pdeo->mutex)) {
+		/* somebody else is doing that, just wait */
+		spin_unlock(&pde->pde_unload_lock);
+		mutex_lock(&pdeo->mutex);
+		spin_lock(&pde->pde_unload_lock);
+		WARN_ON(!list_empty(&pdeo->lh));
+	} else {
+		struct file *file;
+		spin_unlock(&pde->pde_unload_lock);
+		file = pdeo->file;
+		pde->proc_fops->release(file_inode(file), file);
+		spin_lock(&pde->pde_unload_lock);
+		list_del_init(&pdeo->lh);
+	}
+	mutex_unlock(&pdeo->mutex);
+	if (!--pdeo->count)
+		kfree(pdeo);
+}
+
 void proc_entry_rundown(struct proc_dir_entry *de)
 {
 	spin_lock(&de->pde_unload_lock);
  
@@ -173,15 +196,8 @@
  
 	while (!list_empty(&de->pde_openers)) {
 		struct pde_opener *pdeo;
-		struct file *file;
-
 		pdeo = list_first_entry(&de->pde_openers, struct pde_opener, lh);
-		list_del(&pdeo->lh);
-		spin_unlock(&de->pde_unload_lock);
-		file = pdeo->file;
-		de->proc_fops->release(file_inode(file), file);
-		kfree(pdeo);
-		spin_lock(&de->pde_unload_lock);
+		close_pdeo(de, pdeo);
 	}
 	spin_unlock(&de->pde_unload_lock);
 }
@@ -357,6 +373,8 @@
 	spin_lock(&pde->pde_unload_lock);
 	if (rv == 0 && release) {
 		/* To know what to release. */
+		mutex_init(&pdeo->mutex);
+		pdeo->count = 0;
 		pdeo->file = file;
 		/* Strictly for "too late" ->release in proc_reg_release(). */
 		list_add(&pdeo->lh, &pde->pde_openers);
  
  
  
  
  
@@ -367,58 +385,19 @@
 	return rv;
 }
  
-static struct pde_opener *find_pde_opener(struct proc_dir_entry *pde,
-					struct file *file)
-{
-	struct pde_opener *pdeo;
-
-	list_for_each_entry(pdeo, &pde->pde_openers, lh) {
-		if (pdeo->file == file)
-			return pdeo;
-	}
-	return NULL;
-}
-
 static int proc_reg_release(struct inode *inode, struct file *file)
 {
 	struct proc_dir_entry *pde = PDE(inode);
-	int rv = 0;
-	int (*release)(struct inode *, struct file *);
 	struct pde_opener *pdeo;
-
 	spin_lock(&pde->pde_unload_lock);
-	pdeo = find_pde_opener(pde, file);
-	if (pde->pde_users < 0) {
-		/*
-		 * Can't simply exit, __fput() will think that everything is OK,
-		 * and move on to freeing struct file. remove_proc_entry() will
-		 * find slacker in opener's list and will try to do non-trivial
-		 * things with struct file. Therefore, remove opener from list.
-		 *
-		 * But if opener is removed from list, who will ->release it?
-		 */
-		if (pdeo) {
-			list_del(&pdeo->lh);
-			spin_unlock(&pde->pde_unload_lock);
-			rv = pde->proc_fops->release(inode, file);
-			kfree(pdeo);
-		} else
-			spin_unlock(&pde->pde_unload_lock);
-		return rv;
+	list_for_each_entry(pdeo, &pde->pde_openers, lh) {
+		if (pdeo->file == file) {
+			close_pdeo(pde, pdeo);
+			break;
+		}
 	}
-	pde->pde_users++;
-	release = pde->proc_fops->release;
-	if (pdeo) {
-		list_del(&pdeo->lh);
-		kfree(pdeo);
-	}
 	spin_unlock(&pde->pde_unload_lock);
-
-	if (release)
-		rv = release(inode, file);
-
-	unuse_pde(pde);
-	return rv;
+	return 0;
 }
  
 static const struct file_operations proc_reg_file_ops = {
@@ -153,6 +153,8 @@
 struct pde_opener {
 	struct file *file;
 	struct list_head lh;
+	int count;	/* number of threads in close_pdeo() */
+	struct mutex mutex;
 };
  
 ssize_t __proc_file_read(struct file *, char __user *, size_t, loff_t *);
...	...	@@ -156,6 +156,29 @@
156	156	spin_unlock(&pde->pde_unload_lock);
157	157	}
158	158
	159	+/* pde is locked */
	160	+static void close_pdeo(struct proc_dir_entry pde, struct pde_opener pdeo)
	161	+{
	162	+ pdeo->count++;
	163	+ if (!mutex_trylock(&pdeo->mutex)) {
	164	+ /* somebody else is doing that, just wait */
	165	+ spin_unlock(&pde->pde_unload_lock);
	166	+ mutex_lock(&pdeo->mutex);
	167	+ spin_lock(&pde->pde_unload_lock);
	168	+ WARN_ON(!list_empty(&pdeo->lh));
	169	+ } else {
	170	+ struct file *file;
	171	+ spin_unlock(&pde->pde_unload_lock);
	172	+ file = pdeo->file;
	173	+ pde->proc_fops->release(file_inode(file), file);
	174	+ spin_lock(&pde->pde_unload_lock);
	175	+ list_del_init(&pdeo->lh);
	176	+ }
	177	+ mutex_unlock(&pdeo->mutex);
	178	+ if (!--pdeo->count)
	179	+ kfree(pdeo);
	180	+}
	181	+
159	182	void proc_entry_rundown(struct proc_dir_entry *de)
160	183	{
161	184	spin_lock(&de->pde_unload_lock);
162	185
...	...	@@ -173,15 +196,8 @@
173	196
174	197	while (!list_empty(&de->pde_openers)) {
175	198	struct pde_opener *pdeo;
176		- struct file *file;
177		-
178	199	pdeo = list_first_entry(&de->pde_openers, struct pde_opener, lh);
179		- list_del(&pdeo->lh);
180		- spin_unlock(&de->pde_unload_lock);
181		- file = pdeo->file;
182		- de->proc_fops->release(file_inode(file), file);
183		- kfree(pdeo);
184		- spin_lock(&de->pde_unload_lock);
	200	+ close_pdeo(de, pdeo);
185	201	}
186	202	spin_unlock(&de->pde_unload_lock);
187	203	}
...	...	@@ -357,6 +373,8 @@
357	373	spin_lock(&pde->pde_unload_lock);
358	374	if (rv == 0 && release) {
359	375	/* To know what to release. */
	376	+ mutex_init(&pdeo->mutex);
	377	+ pdeo->count = 0;
360	378	pdeo->file = file;
361	379	/* Strictly for "too late" ->release in proc_reg_release(). */
362	380	list_add(&pdeo->lh, &pde->pde_openers);
363	381
364	382
365	383
366	384
367	385
...	...	@@ -367,58 +385,19 @@
367	385	return rv;
368	386	}
369	387
370		-static struct pde_opener find_pde_opener(struct proc_dir_entry pde,
371		- struct file *file)
372		-{
373		- struct pde_opener *pdeo;
374		-
375		- list_for_each_entry(pdeo, &pde->pde_openers, lh) {
376		- if (pdeo->file == file)
377		- return pdeo;
378		- }
379		- return NULL;
380		-}
381		-
382	388	static int proc_reg_release(struct inode inode, struct file file)
383	389	{
384	390	struct proc_dir_entry *pde = PDE(inode);
385		- int rv = 0;
386		- int (release)(struct inode , struct file *);
387	391	struct pde_opener *pdeo;
388		-
389	392	spin_lock(&pde->pde_unload_lock);
390		- pdeo = find_pde_opener(pde, file);
391		- if (pde->pde_users < 0) {
392		- /*
393		- * Can't simply exit, __fput() will think that everything is OK,
394		- * and move on to freeing struct file. remove_proc_entry() will
395		- * find slacker in opener's list and will try to do non-trivial
396		- * things with struct file. Therefore, remove opener from list.
397		- *
398		- * But if opener is removed from list, who will ->release it?
399		- */
400		- if (pdeo) {
401		- list_del(&pdeo->lh);
402		- spin_unlock(&pde->pde_unload_lock);
403		- rv = pde->proc_fops->release(inode, file);
404		- kfree(pdeo);
405		- } else
406		- spin_unlock(&pde->pde_unload_lock);
407		- return rv;
	393	+ list_for_each_entry(pdeo, &pde->pde_openers, lh) {
	394	+ if (pdeo->file == file) {
	395	+ close_pdeo(pde, pdeo);
	396	+ break;
	397	+ }
408	398	}
409		- pde->pde_users++;
410		- release = pde->proc_fops->release;
411		- if (pdeo) {
412		- list_del(&pdeo->lh);
413		- kfree(pdeo);
414		- }
415	399	spin_unlock(&pde->pde_unload_lock);
416		-
417		- if (release)
418		- rv = release(inode, file);
419		-
420		- unuse_pde(pde);
421		- return rv;
	400	+ return 0;
422	401	}
423	402
424	403	static const struct file_operations proc_reg_file_ops = {
...	...	@@ -153,6 +153,8 @@
153	153	struct pde_opener {
154	154	struct file *file;
155	155	struct list_head lh;
	156	+ int count; /* number of threads in close_pdeo() */
	157	+ struct mutex mutex;
156	158	};
157	159
158	160	ssize_t __proc_file_read(struct file , char __user , size_t, loff_t *);