linux-kernel - Re: [PATCH v3 074/110] VFS: replace {, total_}link_count in task

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]

Message-ID: <20150514232148.GA24737@ZenIV.linux.org.uk>
Date:	Fri, 15 May 2015 00:21:49 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Neil Brown <neilb@...e.de>, Christoph Hellwig <hch@...radead.org>,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v3 074/110] VFS: replace {, total_}link_count in
 task_struct with pointer to nameidata

On Mon, May 11, 2015 at 07:07:34PM +0100, Al Viro wrote:
> From: NeilBrown <neilb@...e.de>
> 
> task_struct currently contains two ad-hoc members for use by the VFS:
> link_count and total_link_count.  These are only interesting to fs/namei.c,
> so exposing them explicitly is poor layering.  Incidentally, link_count
> isn't used anymore, so it can just die.
> 
> This patches replaces those with a single pointer to 'struct nameidata'.
> This structure represents the current filename lookup of which
> there can only be one per process, and is a natural place to
> store total_link_count.
> 
> This will allow the current "nameidata" argument to all
> follow_link operations to be removed as current->nameidata
> can be used instead in the _very_ few instances that care about
> it at all.
> 
> As there are occasional circumstances where pathname lookup can
> recurse, such as through kern_path_locked, we always save and old
> current->nameidata (if there is one) when setting a new value, and
> make sure any active link_counts are preserved.
> 
> follow_mount and follow_automount now get a 'struct nameidata *'
> rather than 'int flags' so that they can directly access
> total_link_count, rather than going through 'current'.

FWIW, the mainline semantics for total_link_count is bogus.  Look: we set
it to zero in path_init() (since _way_ back - in fact, 2.4.12 had something like
that in path_walk()).  Now, consider what happens when something like e.g.
NFS referral point crossing does pathname resolution (recursion there is
limited to "no more than once" by referral loop prevention logics in NFS code).
We get the damn thing quietly reset to zero, no matter how many symlinks had
already been crossed.

IMO we should drop that assignment in path_init() and let the logics in
set_nameidata/restore_nameidata do the right thing.  The current semantics
is obviously wrong; we should either count the symlink traversals during
a referral resolution towards the limit or just limit those to 40 independently
from the symlink traversals outside of referral resolution, but resetting the
count to zero as soon as we'd hit the referral is clearly bogus.

I would prefer the "let's count them towards the limit" variant.  Objections?
--
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/