linux-kernel - Re: [PATCH] single_open/seq

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <alpine.LFD.0.999.0708021217370.8258@enigma.security.iitk.ac.in>
Date:	Thu, 2 Aug 2007 12:24:05 +0530 (IST)
From:	Satyam Sharma <satyam@...radead.org>
To:	Alexey Dobriyan <adobriyan@...ru>
cc:	akpm@...l.org, viro@....linux.org.uk, linux-kernel@...r.kernel.org,
	devel@...nvz.org, mingo@...e.hu
Subject: Re: [PATCH] single_open/seq_release leak diagnostics

Hi,


On Tue, 31 Jul 2007, Alexey Dobriyan wrote:

> [...]
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -281,6 +281,13 @@ EXPORT_SYMBOL(seq_lseek);
>  int seq_release(struct inode *inode, struct file *file)
>  {
>  	struct seq_file *m = (struct seq_file *)file->private_data;
> +
> +	if (m->seq_ops_allocated) {
> +		struct dentry *dentry = file->f_dentry;
> +		printk("memory leak: '%.*s'\n",
> +			dentry->d_name.len, dentry->d_name.name);
> +		WARN_ON(1);
> +	}
>  	kfree(m->buf);
>  	kfree(m);
>  	return 0;
> @@ -401,9 +408,12 @@ int single_open(struct file *file, int (*show)(struct seq_file *, void *),
>  		op->stop = single_stop;
>  		op->show = show;
>  		res = seq_open(file, op);
> -		if (!res)
> -			((struct seq_file *)file->private_data)->private = data;
> -		else
> +		if (!res) {
> +			struct seq_file *seq = file->private_data;
> +
> +			seq->private = data;
> +			seq->seq_ops_allocated = 1;
> +		} else
>  			kfree(op);
>  	}
>  	return res;
> @@ -412,8 +422,13 @@ EXPORT_SYMBOL(single_open);
>  
>  int single_release(struct inode *inode, struct file *file)
>  {
> -	const struct seq_operations *op = ((struct seq_file *)file->private_data)->op;
> -	int res = seq_release(inode, file);
> +	struct seq_file *seq = file->private_data;
> +	const struct seq_operations *op = seq->op;
> +	int res;
> +
> +	/* All roads lead to seq_release(), so... */
> +	seq->seq_ops_allocated = 0;
> +	res = seq_release(inode, file);
>  	kfree(op);
>  	return res;
>  }
> --- a/include/linux/seq_file.h
> +++ b/include/linux/seq_file.h
> @@ -22,6 +22,7 @@ struct seq_file {
>  	struct mutex lock;
>  	const struct seq_operations *op;
>  	void *private;
> +	unsigned int seq_ops_allocated:1;
>  };
>  
>  struct seq_operations {

Hmm, curiously, I think this patch just killed the utility of having
single_release() around in the first place :-)

We might as well free the seq_file->op when we detect that we're
leaking it in seq_release() itself. That makes single_release() wholly
redundant to keep, and we can just convert all its users to seq_release()
itself. With less of these around, lesser probability of someone coding
a bug/leak in the first place!


Just my Rs. 0.02,

Satyam
-
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/