linux-kernel - Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 15 Aug 2012 22:16:28 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Cyrill Gorcunov <gorcunov@...nvz.org>
Cc:	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	Alexey Dobriyan <adobriyan@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Pavel Emelyanov <xemul@...allels.com>,
	James Bottomley <jbottomley@...allels.com>,
	Matthew Helsley <matt.helsley@...il.com>
Subject: Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo
 providers

On Wed, Aug 15, 2012 at 01:21:19PM +0400, Cyrill Gorcunov wrote:
> -static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct path *path)
> +static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct file **f_file, struct path *path)

Bloody bad taste, that...  This kind of optional arguments is almost always
a bad sign - tends to happen when you have two barely related functions
crammed into one.  And yes, proc_fd_info() suffers the same braindamage.
Trying to avoid code duplication is generally a good thing, but it's not
always worth doing - less obfuscated code wins.

>  static int seq_show(struct seq_file *m, void *v)
>  {
>  	struct proc_fdinfo *fdinfo = m->private;
> -	seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
> -		   (long long)fdinfo->f_pos,
> -		   fdinfo->f_flags);
> -	return 0;
> +	int ret;
> +
> +	ret = seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
> +			 (long long)fdinfo->f_file->f_pos,
> +			 fdinfo->f_flags);

Realistically, that one is not going to overflow; you are almost certainly
wasting more cycles on that check of !ret just below than you'll save on
not going into ->show_fdinfo() in case of full buffer.

> +	if (!ret && fdinfo->f_file->f_op->show_fdinfo)
> +		ret = fdinfo->f_file->f_op->show_fdinfo(m, fdinfo->f_file);
> +
> +	return ret;
>  }

> +	ret = single_open(file, seq_show, fdinfo);
> +	if (ret) {
> +		put_filp(fdinfo->f_file);

Excuse me?  We should *never* do put_filp() on anything that has already
been opened.  Think what happens if you race with close(); close() would
rip the reference from descriptor table and do fput(), leaving you with
the last reference to that struct file.  You really don't want to just
go and free it.  IOW, that one should be fput().

> +	put_filp(fdinfo->f_file);
Ditto.
--
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/