linux-kernel - Re: [PATCH 3/9] VFS: Introduce a mount context

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: <1494422409.2688.13.camel@redhat.com>
Date:   Wed, 10 May 2017 09:20:09 -0400
From:   Jeff Layton <jlayton@...hat.com>
To:     David Howells <dhowells@...hat.com>,
        Miklos Szeredi <mszeredi@...hat.com>
Cc:     viro <viro@...iv.linux.org.uk>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-nfs@...r.kernel.org, lkml <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/9] VFS: Introduce a mount context

On Wed, 2017-05-10 at 09:05 +0100, David Howells wrote:
> Miklos Szeredi <mszeredi@...hat.com> wrote:
>
> > Possible rule of thumb: use it only at the place where the error
> > originates and not where errors are just passed on.  This would result
> > in at most one report per syscall, normally.
> >

That might be hard to enforce in practice once you get into some
complicated layering. What if we have device_mapper setting this along
with filesystems too? We need clear rules here.

> > And the static string thing that David implemented is also a very good
> > idea, IMO.
> 
> There is an issue with it: it's fine as long as you keep a ref on the module
> that generated it or clear all strings as part of module removal (which the
> mount context in this patchset does).  With the NFS mount context I did, I
> have to keep a ref on the NFS protocol module as well as the NFS filesystem
> module.
> 
> I'm tempted to make it conditionally copy the string using kvasprintf_const()
> - which would also permit format substitution.
> 

On balance, I think this is a reasonable way to pass back detailed
errors. Up until now, we've mostly relied on just printk'ing them. Now
though, a lot of larger machines are running containerized setups. Good
luck scraping dmesg for _your_ error in that situation. There may be
tons of mounts failing all over the place.

That said, I have some concerns here:

What's the lifetime of these strings? Do they just hang around forever
until the process goes away or they're replaced? If this becomes common,
then you could easily end up with an extra string allocation per task in
some cases. That could add up.

One idea might be to always kfree it on syscall entry, and that might
mitigate the problem assuming that not everything is erroring out. Then
you could always do some trivial syscall to clear it manually.

There's also the problem of how these should be formatted. Is English ok
everywhere? Do we need a facility to allow translating these things?
-- 
Jeff Layton <jlayton@...hat.com>