linux-kernel - Re: [Patch] mqueue: fix the bad code in sys_mq

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]

Date:	Wed, 3 Mar 2010 19:54:49 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Am??rico Wang <xiyou.wangcong@...il.com>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Andr?? Goddard Rosa <andre.goddard@...il.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Serge E . Hallyn" <serue@...ibm.com>,
	Cedric Le Goater <clg@...ibm.com>,
	Xiaotian Feng <xtfeng@...il.com>
Subject: Re: [Patch] mqueue: fix the bad code in sys_mq_open()

On Thu, Feb 25, 2010 at 09:40:23PM +0800, Am??rico Wang wrote:
> 
> Fix bad code in ipc/mqueue.c.
> 
> Inspired by Andr?? Goddard Rosa's  patch.
> 
> a) do_create() and do_open() should not release the resources which
>    are not acquired within themselves, it's their caller's work;

Sorry, NAK.  Resources are either consumed by opened file (i.e. struct
file now is the holder of mnt/dentry) or released; in either case,
caller has given them up.  Your variant is broken in its current form
and will be more complicated than existing code if you fix it.

> b) Fix an fd leak;

Fixed by original patch.

> c) The goto label 'out_upsem' doesn't make any sense, rename to
>    'out_unlock';
> 
> d) mntget() should be called before looking up dentry within
>    ->mnt->mnt_root;

Not really, since ->mnt doesn't change (and remains pinned) for the
entire lifetime of ipc_ns.

> e) When dealing with failure case, we should release the resources
>    in a reversed order of acquiring, so reorder the goto labels;
> 
> f) Remove some unused labels due to reorder.

I wouldn't say it's cleaner that way.

Anyway, I've applied the patchset as posted; if you want to do something
else, do that on top of it.  I really object against your (a), the rest
is more or less a matter of taste.  (a) is just plain wrong - we want
uniform behaviour from the caller's POV and it means that mnt/dentry should
always be given up from caller's POV.  Either moved into new struct file,
or dropped.
--
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/