Commit 881adb85 authored by Alexey Dobriyan's avatar Alexey Dobriyan Committed by Linus Torvalds

proc: always do ->release

Current two-stage scheme of removing PDE emphasizes one bug in proc:

		open
				rmmod
				remove_proc_entry
		close

->release won't be called because ->proc_fops were cleared.  In simple
cases it's small memory leak.

For every ->open, ->release has to be done.  List of openers is introduced
which is traversed at remove_proc_entry() if neeeded.

Discussions with Al long ago (sigh).
Signed-off-by: default avatarAlexey Dobriyan <adobriyan@gmail.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: default avatarAndrew Morton <akpm@linux-foundation.org>
Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
parent 6e644c31
...@@ -597,6 +597,7 @@ static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent, ...@@ -597,6 +597,7 @@ static struct proc_dir_entry *__proc_create(struct proc_dir_entry **parent,
ent->pde_users = 0; ent->pde_users = 0;
spin_lock_init(&ent->pde_unload_lock); spin_lock_init(&ent->pde_unload_lock);
ent->pde_unload_completion = NULL; ent->pde_unload_completion = NULL;
INIT_LIST_HEAD(&ent->pde_openers);
out: out:
return ent; return ent;
} }
...@@ -789,6 +790,19 @@ void remove_proc_entry(const char *name, struct proc_dir_entry *parent) ...@@ -789,6 +790,19 @@ void remove_proc_entry(const char *name, struct proc_dir_entry *parent)
spin_unlock(&de->pde_unload_lock); spin_unlock(&de->pde_unload_lock);
continue_removing: continue_removing:
spin_lock(&de->pde_unload_lock);
while (!list_empty(&de->pde_openers)) {
struct pde_opener *pdeo;
pdeo = list_first_entry(&de->pde_openers, struct pde_opener, lh);
list_del(&pdeo->lh);
spin_unlock(&de->pde_unload_lock);
pdeo->release(pdeo->inode, pdeo->file);
kfree(pdeo);
spin_lock(&de->pde_unload_lock);
}
spin_unlock(&de->pde_unload_lock);
if (S_ISDIR(de->mode)) if (S_ISDIR(de->mode))
parent->nlink--; parent->nlink--;
de->nlink = 0; de->nlink = 0;
......
...@@ -126,12 +126,17 @@ static const struct super_operations proc_sops = { ...@@ -126,12 +126,17 @@ static const struct super_operations proc_sops = {
.remount_fs = proc_remount, .remount_fs = proc_remount,
}; };
static void pde_users_dec(struct proc_dir_entry *pde) static void __pde_users_dec(struct proc_dir_entry *pde)
{ {
spin_lock(&pde->pde_unload_lock);
pde->pde_users--; pde->pde_users--;
if (pde->pde_unload_completion && pde->pde_users == 0) if (pde->pde_unload_completion && pde->pde_users == 0)
complete(pde->pde_unload_completion); complete(pde->pde_unload_completion);
}
static void pde_users_dec(struct proc_dir_entry *pde)
{
spin_lock(&pde->pde_unload_lock);
__pde_users_dec(pde);
spin_unlock(&pde->pde_unload_lock); spin_unlock(&pde->pde_unload_lock);
} }
...@@ -318,36 +323,97 @@ static int proc_reg_open(struct inode *inode, struct file *file) ...@@ -318,36 +323,97 @@ static int proc_reg_open(struct inode *inode, struct file *file)
struct proc_dir_entry *pde = PDE(inode); struct proc_dir_entry *pde = PDE(inode);
int rv = 0; int rv = 0;
int (*open)(struct inode *, struct file *); int (*open)(struct inode *, struct file *);
int (*release)(struct inode *, struct file *);
struct pde_opener *pdeo;
/*
* What for, you ask? Well, we can have open, rmmod, remove_proc_entry
* sequence. ->release won't be called because ->proc_fops will be
* cleared. Depending on complexity of ->release, consequences vary.
*
* We can't wait for mercy when close will be done for real, it's
* deadlockable: rmmod foo </proc/foo . So, we're going to do ->release
* by hand in remove_proc_entry(). For this, save opener's credentials
* for later.
*/
pdeo = kmalloc(sizeof(struct pde_opener), GFP_KERNEL);
if (!pdeo)
return -ENOMEM;
spin_lock(&pde->pde_unload_lock); spin_lock(&pde->pde_unload_lock);
if (!pde->proc_fops) { if (!pde->proc_fops) {
spin_unlock(&pde->pde_unload_lock); spin_unlock(&pde->pde_unload_lock);
kfree(pdeo);
return rv; return rv;
} }
pde->pde_users++; pde->pde_users++;
open = pde->proc_fops->open; open = pde->proc_fops->open;
release = pde->proc_fops->release;
spin_unlock(&pde->pde_unload_lock); spin_unlock(&pde->pde_unload_lock);
if (open) if (open)
rv = open(inode, file); rv = open(inode, file);
pde_users_dec(pde); spin_lock(&pde->pde_unload_lock);
if (rv == 0 && release) {
/* To know what to release. */
pdeo->inode = inode;
pdeo->file = file;
/* Strictly for "too late" ->release in proc_reg_release(). */
pdeo->release = release;
list_add(&pdeo->lh, &pde->pde_openers);
} else
kfree(pdeo);
__pde_users_dec(pde);
spin_unlock(&pde->pde_unload_lock);
return rv; return rv;
} }
static struct pde_opener *find_pde_opener(struct proc_dir_entry *pde,
struct inode *inode, struct file *file)
{
struct pde_opener *pdeo;
list_for_each_entry(pdeo, &pde->pde_openers, lh) {
if (pdeo->inode == inode && pdeo->file == file)
return pdeo;
}
return NULL;
}
static int proc_reg_release(struct inode *inode, struct file *file) static int proc_reg_release(struct inode *inode, struct file *file)
{ {
struct proc_dir_entry *pde = PDE(inode); struct proc_dir_entry *pde = PDE(inode);
int rv = 0; int rv = 0;
int (*release)(struct inode *, struct file *); int (*release)(struct inode *, struct file *);
struct pde_opener *pdeo;
spin_lock(&pde->pde_unload_lock); spin_lock(&pde->pde_unload_lock);
pdeo = find_pde_opener(pde, inode, file);
if (!pde->proc_fops) { if (!pde->proc_fops) {
/*
* 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 = pdeo->release(inode, file);
kfree(pdeo);
} else
spin_unlock(&pde->pde_unload_lock); spin_unlock(&pde->pde_unload_lock);
return rv; return rv;
} }
pde->pde_users++; pde->pde_users++;
release = pde->proc_fops->release; release = pde->proc_fops->release;
if (pdeo) {
list_del(&pdeo->lh);
kfree(pdeo);
}
spin_unlock(&pde->pde_unload_lock); spin_unlock(&pde->pde_unload_lock);
if (release) if (release)
......
...@@ -89,3 +89,10 @@ struct dentry *proc_lookup_de(struct proc_dir_entry *de, struct inode *ino, ...@@ -89,3 +89,10 @@ struct dentry *proc_lookup_de(struct proc_dir_entry *de, struct inode *ino,
struct dentry *dentry); struct dentry *dentry);
int proc_readdir_de(struct proc_dir_entry *de, struct file *filp, void *dirent, int proc_readdir_de(struct proc_dir_entry *de, struct file *filp, void *dirent,
filldir_t filldir); filldir_t filldir);
struct pde_opener {
struct inode *inode;
struct file *file;
int (*release)(struct inode *, struct file *);
struct list_head lh;
};
...@@ -79,6 +79,7 @@ struct proc_dir_entry { ...@@ -79,6 +79,7 @@ struct proc_dir_entry {
int pde_users; /* number of callers into module in progress */ int pde_users; /* number of callers into module in progress */
spinlock_t pde_unload_lock; /* proc_fops checks and pde_users bumps */ spinlock_t pde_unload_lock; /* proc_fops checks and pde_users bumps */
struct completion *pde_unload_completion; struct completion *pde_unload_completion;
struct list_head pde_openers; /* who did ->open, but not ->release */
}; };
struct kcore_list { struct kcore_list {
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment