* Re: [PATCH v2 6/5] statx: add STATX_RESULT_MASK flag
From: David Howells @ 2018-10-19 15:59 UTC (permalink / raw)
To: Miklos Szeredi
Cc: dhowells, linux-fsdevel, linux-kernel, linux-api, Michael Kerrisk,
Andreas Dilger, Florian Weimer, Amir Goldstein
In-Reply-To: <20181019143932.6697-1-mszeredi@redhat.com>
Miklos Szeredi <mszeredi@redhat.com> wrote:
> FUSE needs this, because it uses legacy inode initialization, that doesn't
> return a result_mask, so needs a refresh when caller asks for it with
> statx().
Can't you just make it up in fuse? Presumably, fuse doesn't support any of
the non-basic statx fields either?
> It might make sense later to promote this to a proper statx mask flag and
> return it in stx_mask to userspace.
That sounds kind of recursive - a bit in stx_mask would be saying whether or
not stx_mask can be used.
Besides, what would it mean if that bit says you can't use stx_mask? None of
the stx_* fields are valid?
> +#define STATX_RESULT_MASK STATX__RESERVED
Please don't use that bit.
Sorry, this patch doesn't make sense. Just set result_mask to
STATX_BASIC_STATS in fuse_fill_attr().
David
^ permalink raw reply
* Re: [PATCH v2 2/5] uapi: deprecate STATX_ALL
From: Miklos Szeredi @ 2018-10-19 17:30 UTC (permalink / raw)
To: David Howells
Cc: Miklos Szeredi, linux-fsdevel, linux-kernel, Linux API,
Michael Kerrisk, Andreas Dilger, Florian Weimer, Amir Goldstein
In-Reply-To: <17323.1539964061@warthog.procyon.org.uk>
On Fri, Oct 19, 2018 at 5:47 PM, David Howells <dhowells@redhat.com> wrote:
> Miklos Szeredi <miklos@szeredi.hu> wrote:
>
>> What is misleading about this?
>
> The manpage says:
>
> STATX_ALL [All currently available fields]
Ah, the manpage needs to be fixed, obviously.
>
>> If you feel confused by this comment, then maybe I should just drop that part.
>
> I think that would be better. Don't try to give it a definition.
Okay.
Thanks,
Miklos
^ permalink raw reply
* Re: [PATCH v2 6/5] statx: add STATX_RESULT_MASK flag
From: Miklos Szeredi @ 2018-10-19 17:42 UTC (permalink / raw)
To: David Howells
Cc: Miklos Szeredi, linux-fsdevel, linux-kernel, Linux API,
Michael Kerrisk, Andreas Dilger, Florian Weimer, Amir Goldstein
In-Reply-To: <18142.1539964788@warthog.procyon.org.uk>
On Fri, Oct 19, 2018 at 5:59 PM, David Howells <dhowells@redhat.com> wrote:
> Miklos Szeredi <mszeredi@redhat.com> wrote:
>
>> FUSE needs this, because it uses legacy inode initialization, that doesn't
>> return a result_mask, so needs a refresh when caller asks for it with
>> statx().
>
> Can't you just make it up in fuse? Presumably, fuse doesn't support any of
> the non-basic statx fields either?
This is very much about the basic statx fields. I.e. what does
result_mask == STATX_BASIC_STATS means?
a) this is a legacy stat() implementation that doesn't fill in the
result_mask properly
b) this is an up-todate stat() implementation that does fill the
result_mask properly and all fields are valid
There's no way to tell which one it is, and no way to request that
filesystem try b) even if it's more costly.
>> It might make sense later to promote this to a proper statx mask flag and
>> return it in stx_mask to userspace.
>
> That sounds kind of recursive - a bit in stx_mask would be saying whether or
> not stx_mask can be used.
Yes. See above.
>> +#define STATX_RESULT_MASK STATX__RESERVED
>
> Please don't use that bit.
Using it internally is perfectly harmless. If we'll need to extend
statx in the future and make use of this flag externally, then we can
easily move the internal flag somewhere else (e.g. extend request_mask
to 64bit, which we'll probably need to do anyway in that case).
Thanks,
Miklos
^ permalink raw reply
* Re: [PATCH v2 5/5] nfs: don't clear STATX_ATIME from result_mask
From: Miklos Szeredi @ 2018-10-19 17:46 UTC (permalink / raw)
To: Trond Myklebust
Cc: mszeredi@redhat.com, linux-fsdevel@vger.kernel.org,
amir73il@gmail.com, linux-kernel@vger.kernel.org,
adilger@dilger.ca, mtk.manpages@gmail.com, fw@deneb.enyo.de,
linux-api@vger.kernel.org, dhowells@redhat.com
In-Reply-To: <2ca365ec330cf918188e8c47b26e741073382d9d.camel@primarydata.com>
On Fri, Oct 19, 2018 at 5:52 PM, Trond Myklebust
<trondmy@hammerspace.com> wrote:
> On Fri, 2018-10-19 at 14:20 +0200, Miklos Szeredi wrote:
>> As per statx(2) man page only clear out flags that are unsupported by
>> the
>> fs or have an unrepresentable value. Atime is supported by NFS as
>> long as
>> it's supported on the server.
>>
>> So the STATX_ATIME flag should not be cleared in the result_mask if
>> the
>> operation was requested on a MNT_NOATIME or MNT_NODIRATIME mount.
>>
>> This patch doesn't change the revalidation algorithm in any way, just
>> the
>> clearing of flags in stat->result_mask.
>>
>> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
>> Fixes: 9ccee940bd5b ("Support statx() mask and query flags
>> parameters")
>> Cc: Trond Myklebust <trond.myklebust@primarydata.com>
>> ---
>> fs/nfs/inode.c | 5 +----
>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
>> index b65aee481d13..34bb3e591709 100644
>> --- a/fs/nfs/inode.c
>> +++ b/fs/nfs/inode.c
>> @@ -811,7 +811,7 @@ int nfs_getattr(const struct path *path, struct
>> kstat *stat,
>> if (!(request_mask &
>> (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME|
>> STATX_MTIME|STATX_UID|STATX_GID
>> |
>> STATX_SIZE|STATX_BLOCKS)))
>> - goto out_no_revalidate;
>> + goto out_no_update;
>>
>> /* Check whether the cached attributes are stale */
>> do_update |= force_sync || nfs_attribute_cache_expired(inode);
>> @@ -833,9 +833,6 @@ int nfs_getattr(const struct path *path, struct
>> kstat *stat,
>> goto out;
>> } else
>> nfs_readdirplus_parent_cache_hit(path->dentry);
>> -out_no_revalidate:
>> - /* Only return attributes that were revalidated. */
>> - stat->result_mask &= request_mask;
>> out_no_update:
>> generic_fillattr(inode, stat);
>> stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
>
> NACK.
>
> When we don't revalidate the attribute, then the content of the field
> contains junk values. The above code is very intentional.
How is it then that only STATX_ATIME is cleared and not the other fields?
Note: junk != stale. The statx definition doesn't talk about the
fields being up-to-date, except for AT_STATX_FORCE_SYNC, so stale
attributes are okay, and do not warrant clearing the result_mask.
Thanks,
Miklos
^ permalink raw reply
* Re: [PATCH v2 5/5] nfs: don't clear STATX_ATIME from result_mask
From: Trond Myklebust @ 2018-10-19 18:14 UTC (permalink / raw)
To: miklos@szeredi.hu
Cc: linux-kernel@vger.kernel.org, amir73il@gmail.com,
mszeredi@redhat.com, linux-api@vger.kernel.org, adilger@dilger.ca,
dhowells@redhat.com, fw@deneb.enyo.de, mtk.manpages@gmail.com,
linux-fsdevel@vger.kernel.org
In-Reply-To: <CAJfpegtq1+vZ+uXwzFs20GeSPCR9UbtqD5-fOn8ynoJUJYx25g@mail.gmail.com>
On Fri, 2018-10-19 at 19:46 +0200, Miklos Szeredi wrote:
> On Fri, Oct 19, 2018 at 5:52 PM, Trond Myklebust
> <trondmy@hammerspace.com> wrote:
> > On Fri, 2018-10-19 at 14:20 +0200, Miklos Szeredi wrote:
> > > As per statx(2) man page only clear out flags that are
> > > unsupported by
> > > the
> > > fs or have an unrepresentable value. Atime is supported by NFS
> > > as
> > > long as
> > > it's supported on the server.
> > >
> > > So the STATX_ATIME flag should not be cleared in the result_mask
> > > if
> > > the
> > > operation was requested on a MNT_NOATIME or MNT_NODIRATIME mount.
> > >
> > > This patch doesn't change the revalidation algorithm in any way,
> > > just
> > > the
> > > clearing of flags in stat->result_mask.
> > >
> > > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > > Fixes: 9ccee940bd5b ("Support statx() mask and query flags
> > > parameters")
> > > Cc: Trond Myklebust <trond.myklebust@primarydata.com>
> > > ---
> > > fs/nfs/inode.c | 5 +----
> > > 1 file changed, 1 insertion(+), 4 deletions(-)
> > >
> > > diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> > > index b65aee481d13..34bb3e591709 100644
> > > --- a/fs/nfs/inode.c
> > > +++ b/fs/nfs/inode.c
> > > @@ -811,7 +811,7 @@ int nfs_getattr(const struct path *path,
> > > struct
> > > kstat *stat,
> > > if (!(request_mask &
> > > (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME|
> > > STATX_MTIME|STATX_UID|STATX
> > > _GID
> > > >
> > >
> > > STATX_SIZE|STATX_BLOCKS)))
> > > - goto out_no_revalidate;
> > > + goto out_no_update;
> > >
> > > /* Check whether the cached attributes are stale */
> > > do_update |= force_sync ||
> > > nfs_attribute_cache_expired(inode);
> > > @@ -833,9 +833,6 @@ int nfs_getattr(const struct path *path,
> > > struct
> > > kstat *stat,
> > > goto out;
> > > } else
> > > nfs_readdirplus_parent_cache_hit(path->dentry);
> > > -out_no_revalidate:
> > > - /* Only return attributes that were revalidated. */
> > > - stat->result_mask &= request_mask;
> > > out_no_update:
> > > generic_fillattr(inode, stat);
> > > stat->ino = nfs_compat_user_ino64(NFS_FILEID(inode));
> >
> > NACK.
> >
> > When we don't revalidate the attribute, then the content of the
> > field
> > contains junk values. The above code is very intentional.
>
> How is it then that only STATX_ATIME is cleared and not the other
> fields?
It isn't just the atime. We can also fail to revalidate the ctime and
mtime if they are not being requested by the user.
>
> Note: junk != stale. The statx definition doesn't talk about the
> fields being up-to-date, except for AT_STATX_FORCE_SYNC, so stale
> attributes are okay, and do not warrant clearing the result_mask.
>
I disagree. stale == junk here, because the default of
AT_STATX_SYNC_AS_STAT is described by the manpage as "Do whatever
stat(2) does." which this is not.
The default behaviour for "stat(2)" is to revalidate attributes that we
know or suspect are stale. We never knowingly return stale attributes.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 02/10] audit: add container id
From: Paul Moore @ 2018-10-19 19:38 UTC (permalink / raw)
To: rgb
Cc: containers, linux-api, linux-audit, linux-fsdevel, linux-kernel,
netdev, netfilter-devel, ebiederm, luto, carlos, dhowells, viro,
simo, Eric Paris, Serge Hallyn
In-Reply-To: <12396e378a78ee8dd38c75f7730d67d8fbb08e02.1533065887.git.rgb@redhat.com>
On Tue, Jul 31, 2018 at 4:11 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> Implement the proc fs write to set the audit container identifier of a
> process, emitting an AUDIT_CONTAINER_OP record to document the event.
>
> This is a write from the container orchestrator task to a proc entry of
> the form /proc/PID/audit_containerid where PID is the process ID of the
> newly created task that is to become the first task in a container, or
> an additional task added to a container.
>
> The write expects up to a u64 value (unset: 18446744073709551615).
>
> The writer must have capability CAP_AUDIT_CONTROL.
>
> This will produce a record such as this:
> type=CONTAINER_ID msg=audit(2018-06-06 12:39:29.636:26949) : op=set opid=2209 old-contid=18446744073709551615 contid=123456 pid=628 auid=root uid=root tty=ttyS0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=bash exe=/usr/bin/bash res=yes
You need to update the record type in the example above.
> The "op" field indicates an initial set. The "pid" to "ses" fields are
> the orchestrator while the "opid" field is the object's PID, the process
> being "contained". Old and new audit container identifier values are
> given in the "contid" fields, while res indicates its success.
I understand Steve's concern around the "op" field, but I think it
might be a bit premature to think we might not need to do some sort of
audit container ID management in the future that would want to make
use of the CONTAINER_OP message type. I would like to see the "op"
field preserved.
> It is not permitted to unset the audit container identifier.
> A child inherits its parent's audit container identifier.
>
> See: https://github.com/linux-audit/audit-kernel/issues/90
> See: https://github.com/linux-audit/audit-userspace/issues/51
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> Acked-by: Steve Grubb <sgrubb@redhat.com>
> ---
> fs/proc/base.c | 37 +++++++++++++++++++++++++
> include/linux/audit.h | 24 ++++++++++++++++
> include/uapi/linux/audit.h | 2 ++
> kernel/auditsc.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 131 insertions(+)
...
> @@ -2112,6 +2114,72 @@ int audit_set_loginuid(kuid_t loginuid)
> }
>
> /**
> + * audit_set_contid - set current task's audit_context contid
> + * @contid: contid value
> + *
> + * Returns 0 on success, -EPERM on permission failure.
> + *
> + * Called (set) from fs/proc/base.c::proc_contid_write().
> + */
> +int audit_set_contid(struct task_struct *task, u64 contid)
> +{
> + u64 oldcontid;
> + int rc = 0;
> + struct audit_buffer *ab;
> + uid_t uid;
> + struct tty_struct *tty;
> + char comm[sizeof(current->comm)];
> +
> + task_lock(task);
> + /* Can't set if audit disabled */
> + if (!task->audit) {
> + task_unlock(task);
> + return -ENOPROTOOPT;
> + }
> + oldcontid = audit_get_contid(task);
> + read_lock(&tasklist_lock);
I assume lockdep was happy with nesting the tasklist_lock inside the task lock?
> + /* Don't allow the audit containerid to be unset */
> + if (!audit_contid_valid(contid))
> + rc = -EINVAL;
> + /* if we don't have caps, reject */
> + else if (!capable(CAP_AUDIT_CONTROL))
> + rc = -EPERM;
> + /* if task has children or is not single-threaded, deny */
> + else if (!list_empty(&task->children))
> + rc = -EBUSY;
> + else if (!(thread_group_leader(task) && thread_group_empty(task)))
> + rc = -EALREADY;
> + read_unlock(&tasklist_lock);
> + if (!rc)
> + task->audit->contid = contid;
> + task_unlock(task);
> +
> + if (!audit_enabled)
> + return rc;
> +
> + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_OP);
> + if (!ab)
> + return rc;
> +
> + uid = from_kuid(&init_user_ns, task_uid(current));
> + tty = audit_get_tty(current);
> + audit_log_format(ab, "op=set opid=%d old-contid=%llu contid=%llu pid=%d uid=%u auid=%u tty=%s ses=%u",
> + task_tgid_nr(task), oldcontid, contid,
> + task_tgid_nr(current), uid,
> + from_kuid(&init_user_ns, audit_get_loginuid(current)),
> + tty ? tty_name(tty) : "(none)",
> + audit_get_sessionid(current));
> + audit_put_tty(tty);
> + audit_log_task_context(ab);
> + audit_log_format(ab, " comm=");
> + audit_log_untrustedstring(ab, get_task_comm(comm, current));
> + audit_log_d_path_exe(ab, current->mm);
> + audit_log_format(ab, " res=%d", !rc);
> + audit_log_end(ab);
> + return rc;
> +}
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 02/10] audit: add container id
From: Paul Moore @ 2018-10-19 19:40 UTC (permalink / raw)
To: rgb
Cc: containers, linux-api, linux-audit, linux-fsdevel, linux-kernel,
netdev, netfilter-devel, ebiederm, luto, carlos, dhowells, viro,
simo, Eric Paris, Serge Hallyn
In-Reply-To: <CAHC9VhTT7n4wDoaDGFXAK+wtcsMDDMRkQVCQ2jNYLKSFqFc+bA@mail.gmail.com>
Ooops, I hit send prematurely on this :/ My comments below should
stand, but for things like this I usually try to get through the
entire patchset before sending my comments as later patches can affect
my comments on the earlier patches.
On Fri, Oct 19, 2018 at 3:38 PM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jul 31, 2018 at 4:11 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >
> > Implement the proc fs write to set the audit container identifier of a
> > process, emitting an AUDIT_CONTAINER_OP record to document the event.
> >
> > This is a write from the container orchestrator task to a proc entry of
> > the form /proc/PID/audit_containerid where PID is the process ID of the
> > newly created task that is to become the first task in a container, or
> > an additional task added to a container.
> >
> > The write expects up to a u64 value (unset: 18446744073709551615).
> >
> > The writer must have capability CAP_AUDIT_CONTROL.
> >
> > This will produce a record such as this:
> > type=CONTAINER_ID msg=audit(2018-06-06 12:39:29.636:26949) : op=set opid=2209 old-contid=18446744073709551615 contid=123456 pid=628 auid=root uid=root tty=ttyS0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=bash exe=/usr/bin/bash res=yes
>
> You need to update the record type in the example above.
>
> > The "op" field indicates an initial set. The "pid" to "ses" fields are
> > the orchestrator while the "opid" field is the object's PID, the process
> > being "contained". Old and new audit container identifier values are
> > given in the "contid" fields, while res indicates its success.
>
> I understand Steve's concern around the "op" field, but I think it
> might be a bit premature to think we might not need to do some sort of
> audit container ID management in the future that would want to make
> use of the CONTAINER_OP message type. I would like to see the "op"
> field preserved.
>
> > It is not permitted to unset the audit container identifier.
> > A child inherits its parent's audit container identifier.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/90
> > See: https://github.com/linux-audit/audit-userspace/issues/51
> > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > Acked-by: Serge Hallyn <serge@hallyn.com>
> > Acked-by: Steve Grubb <sgrubb@redhat.com>
> > ---
> > fs/proc/base.c | 37 +++++++++++++++++++++++++
> > include/linux/audit.h | 24 ++++++++++++++++
> > include/uapi/linux/audit.h | 2 ++
> > kernel/auditsc.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 131 insertions(+)
>
> ...
>
> > @@ -2112,6 +2114,72 @@ int audit_set_loginuid(kuid_t loginuid)
> > }
> >
> > /**
> > + * audit_set_contid - set current task's audit_context contid
> > + * @contid: contid value
> > + *
> > + * Returns 0 on success, -EPERM on permission failure.
> > + *
> > + * Called (set) from fs/proc/base.c::proc_contid_write().
> > + */
> > +int audit_set_contid(struct task_struct *task, u64 contid)
> > +{
> > + u64 oldcontid;
> > + int rc = 0;
> > + struct audit_buffer *ab;
> > + uid_t uid;
> > + struct tty_struct *tty;
> > + char comm[sizeof(current->comm)];
> > +
> > + task_lock(task);
> > + /* Can't set if audit disabled */
> > + if (!task->audit) {
> > + task_unlock(task);
> > + return -ENOPROTOOPT;
> > + }
> > + oldcontid = audit_get_contid(task);
> > + read_lock(&tasklist_lock);
>
> I assume lockdep was happy with nesting the tasklist_lock inside the task lock?
>
> > + /* Don't allow the audit containerid to be unset */
> > + if (!audit_contid_valid(contid))
> > + rc = -EINVAL;
> > + /* if we don't have caps, reject */
> > + else if (!capable(CAP_AUDIT_CONTROL))
> > + rc = -EPERM;
> > + /* if task has children or is not single-threaded, deny */
> > + else if (!list_empty(&task->children))
> > + rc = -EBUSY;
> > + else if (!(thread_group_leader(task) && thread_group_empty(task)))
> > + rc = -EALREADY;
> > + read_unlock(&tasklist_lock);
> > + if (!rc)
> > + task->audit->contid = contid;
> > + task_unlock(task);
> > +
> > + if (!audit_enabled)
> > + return rc;
> > +
> > + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_OP);
> > + if (!ab)
> > + return rc;
> > +
> > + uid = from_kuid(&init_user_ns, task_uid(current));
> > + tty = audit_get_tty(current);
> > + audit_log_format(ab, "op=set opid=%d old-contid=%llu contid=%llu pid=%d uid=%u auid=%u tty=%s ses=%u",
> > + task_tgid_nr(task), oldcontid, contid,
> > + task_tgid_nr(current), uid,
> > + from_kuid(&init_user_ns, audit_get_loginuid(current)),
> > + tty ? tty_name(tty) : "(none)",
> > + audit_get_sessionid(current));
> > + audit_put_tty(tty);
> > + audit_log_task_context(ab);
> > + audit_log_format(ab, " comm=");
> > + audit_log_untrustedstring(ab, get_task_comm(comm, current));
> > + audit_log_d_path_exe(ab, current->mm);
> > + audit_log_format(ab, " res=%d", !rc);
> > + audit_log_end(ab);
> > + return rc;
> > +}
>
> --
> paul moore
> www.paul-moore.com
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v2 5/5] nfs: don't clear STATX_ATIME from result_mask
From: Miklos Szeredi @ 2018-10-19 20:48 UTC (permalink / raw)
To: Trond Myklebust
Cc: linux-kernel@vger.kernel.org, amir73il@gmail.com,
mszeredi@redhat.com, linux-api@vger.kernel.org, adilger@dilger.ca,
dhowells@redhat.com, fw@deneb.enyo.de, mtk.manpages@gmail.com,
linux-fsdevel@vger.kernel.org
In-Reply-To: <05184f29b5778563519174769c675c5f6c5b2dd3.camel@hammerspace.com>
On Fri, Oct 19, 2018 at 8:14 PM, Trond Myklebust
<trondmy@hammerspace.com> wrote:
> On Fri, 2018-10-19 at 19:46 +0200, Miklos Szeredi wrote:
>> How is it then that only STATX_ATIME is cleared and not the other
>> fields?
>
> It isn't just the atime. We can also fail to revalidate the ctime and
> mtime if they are not being requested by the user.
>
>>
>> Note: junk != stale. The statx definition doesn't talk about the
>> fields being up-to-date, except for AT_STATX_FORCE_SYNC, so stale
>> attributes are okay, and do not warrant clearing the result_mask.
>>
>
> I disagree. stale == junk here, because the default of
> AT_STATX_SYNC_AS_STAT is described by the manpage as "Do whatever
> stat(2) does." which this is not.
Ah, you are talking about this:
/* Is the user requesting attributes that might need revalidation? */
if (!(request_mask & (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME|
STATX_MTIME|STATX_UID|STATX_GID|
STATX_SIZE|STATX_BLOCKS)))
goto out_no_update;
Well, if this is triggered for statx(..., STATX_ATIME,
AT_STATX_SYNC_AS_STAT) and MNT_NOATIME, then yes, result will be junk.
Which means that the code is wrong, it shouldn't do that.
Otherwise (if something other than STATX_ATIME or STATX_INO or
STATX_TYPE is given as well) it *will* do the same thing as what
stat(2) does, so in that case STATX_ATIME should not be cleared (yet
it is cleared).
I can do a patch, but not tonight...
Thanks,
Miklos
Thanks,
Miklos
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 02/10] audit: add container id
From: Richard Guy Briggs @ 2018-10-19 21:50 UTC (permalink / raw)
To: Paul Moore
Cc: containers, linux-api, linux-audit, linux-fsdevel, linux-kernel,
netdev, netfilter-devel, ebiederm, luto, carlos, dhowells, viro,
simo, Eric Paris, Serge Hallyn
In-Reply-To: <CAHC9VhTT7n4wDoaDGFXAK+wtcsMDDMRkQVCQ2jNYLKSFqFc+bA@mail.gmail.com>
On 2018-10-19 15:38, Paul Moore wrote:
> On Tue, Jul 31, 2018 at 4:11 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >
> > Implement the proc fs write to set the audit container identifier of a
> > process, emitting an AUDIT_CONTAINER_OP record to document the event.
> >
> > This is a write from the container orchestrator task to a proc entry of
> > the form /proc/PID/audit_containerid where PID is the process ID of the
> > newly created task that is to become the first task in a container, or
> > an additional task added to a container.
> >
> > The write expects up to a u64 value (unset: 18446744073709551615).
> >
> > The writer must have capability CAP_AUDIT_CONTROL.
> >
> > This will produce a record such as this:
> > type=CONTAINER_ID msg=audit(2018-06-06 12:39:29.636:26949) : op=set opid=2209 old-contid=18446744073709551615 contid=123456 pid=628 auid=root uid=root tty=ttyS0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=bash exe=/usr/bin/bash res=yes
>
> You need to update the record type in the example above.
Yup, thanks.
> > The "op" field indicates an initial set. The "pid" to "ses" fields are
> > the orchestrator while the "opid" field is the object's PID, the process
> > being "contained". Old and new audit container identifier values are
> > given in the "contid" fields, while res indicates its success.
>
> I understand Steve's concern around the "op" field, but I think it
> might be a bit premature to think we might not need to do some sort of
> audit container ID management in the future that would want to make
> use of the CONTAINER_OP message type. I would like to see the "op"
> field preserved.
I strongly agree.
> > It is not permitted to unset the audit container identifier.
> > A child inherits its parent's audit container identifier.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/90
> > See: https://github.com/linux-audit/audit-userspace/issues/51
> > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > Acked-by: Serge Hallyn <serge@hallyn.com>
> > Acked-by: Steve Grubb <sgrubb@redhat.com>
> > ---
> > fs/proc/base.c | 37 +++++++++++++++++++++++++
> > include/linux/audit.h | 24 ++++++++++++++++
> > include/uapi/linux/audit.h | 2 ++
> > kernel/auditsc.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 131 insertions(+)
>
> ...
>
> > @@ -2112,6 +2114,72 @@ int audit_set_loginuid(kuid_t loginuid)
> > }
> >
> > /**
> > + * audit_set_contid - set current task's audit_context contid
> > + * @contid: contid value
> > + *
> > + * Returns 0 on success, -EPERM on permission failure.
> > + *
> > + * Called (set) from fs/proc/base.c::proc_contid_write().
> > + */
> > +int audit_set_contid(struct task_struct *task, u64 contid)
> > +{
> > + u64 oldcontid;
> > + int rc = 0;
> > + struct audit_buffer *ab;
> > + uid_t uid;
> > + struct tty_struct *tty;
> > + char comm[sizeof(current->comm)];
> > +
> > + task_lock(task);
> > + /* Can't set if audit disabled */
> > + if (!task->audit) {
> > + task_unlock(task);
> > + return -ENOPROTOOPT;
> > + }
> > + oldcontid = audit_get_contid(task);
> > + read_lock(&tasklist_lock);
>
> I assume lockdep was happy with nesting the tasklist_lock inside the task lock?
Yup, I had gone through the logic and at first I had doubts, but the
function comments and other usage reassured me (as well as in-kernel
lock checks on boot) that this was the right order and approach.
> > + /* Don't allow the audit containerid to be unset */
> > + if (!audit_contid_valid(contid))
> > + rc = -EINVAL;
> > + /* if we don't have caps, reject */
> > + else if (!capable(CAP_AUDIT_CONTROL))
> > + rc = -EPERM;
> > + /* if task has children or is not single-threaded, deny */
> > + else if (!list_empty(&task->children))
> > + rc = -EBUSY;
> > + else if (!(thread_group_leader(task) && thread_group_empty(task)))
> > + rc = -EALREADY;
> > + read_unlock(&tasklist_lock);
> > + if (!rc)
> > + task->audit->contid = contid;
> > + task_unlock(task);
> > +
> > + if (!audit_enabled)
> > + return rc;
> > +
> > + ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONTAINER_OP);
> > + if (!ab)
> > + return rc;
> > +
> > + uid = from_kuid(&init_user_ns, task_uid(current));
> > + tty = audit_get_tty(current);
> > + audit_log_format(ab, "op=set opid=%d old-contid=%llu contid=%llu pid=%d uid=%u auid=%u tty=%s ses=%u",
> > + task_tgid_nr(task), oldcontid, contid,
> > + task_tgid_nr(current), uid,
> > + from_kuid(&init_user_ns, audit_get_loginuid(current)),
> > + tty ? tty_name(tty) : "(none)",
> > + audit_get_sessionid(current));
> > + audit_put_tty(tty);
> > + audit_log_task_context(ab);
> > + audit_log_format(ab, " comm=");
> > + audit_log_untrustedstring(ab, get_task_comm(comm, current));
> > + audit_log_d_path_exe(ab, current->mm);
> > + audit_log_format(ab, " res=%d", !rc);
> > + audit_log_end(ab);
> > + return rc;
> > +}
>
> --
> paul moore
> www.paul-moore.com
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: [PATCH 1/3] printk: Introduce per-console loglevel setting
From: Calvin Owens @ 2018-10-19 22:03 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt,
linux-api@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, Kernel Team
In-Reply-To: <20181019000403.GB877@jagdpanzerIV>
On Friday 10/19 at 09:04 +0900, Sergey Senozhatsky wrote:
> On (09/28/17 17:43), Calvin Owens wrote:
> > Not all consoles are created equal: depending on the actual hardware,
> > the latency of a printk() call can vary dramatically. The worst examples
> > are serial consoles, where it can spin for tens of milliseconds banging
> > the UART to emit a message, which can cause application-level problems
> > when the kernel spews onto the console.
> >
> > At Facebook we use netconsole to monitor our fleet, but we still have
> > serial consoles attached on each host for live debugging, and the latter
> > has caused problems. An obvious solution is to disable the kernel
> > console output to ttyS0, but this makes live debugging frustrating,
> > since crashes become silent and opaque to the ttyS0 user. Enabling it on
> > the fly when needed isn't feasible, since boxes you need to debug via
> > serial are likely to be borked in ways that make this impossible.
> >
> > That puts us between a rock and a hard place: we'd love to set
> > kernel.printk to KERN_INFO and get all the logs. But while netconsole is
> > fast enough to permit that without perturbing userspace, ttyS0 is not,
> > and we're forced to limit console logging to KERN_WARNING and higher.
> >
> > This patch introduces a new per-console loglevel setting, and changes
> > console_unlock() to use max(global_level, per_console_level) when
> > deciding whether or not to emit a given log message.
> >
> > This lets us have our cake and eat it too: instead of being forced to
> > limit all consoles verbosity based on the speed of the slowest one, we
> > can "promote" the faster console while still using a conservative system
> > loglevel setting to avoid disturbing applications.
>
> Hi Calvin,
>
> Do you have time to address the review feedback and re-spin v2?
Hi Sergey,
It's in-progress, I'm sorry it hasn't happened sooner.
By embedding the kobject in the console struct, we end up needing to refactor
the console drivers to use the kobject refcount instead of simply calling
kfree(), which is what I'm working on. It ends up being tedious but not
particularly complicated, my goal is to have this up soon :)
Thanks,
Calvin
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 01/10] audit: collect audit task parameters
From: Paul Moore @ 2018-10-19 23:15 UTC (permalink / raw)
To: rgb
Cc: containers, linux-api, linux-audit, linux-fsdevel, linux-kernel,
netdev, netfilter-devel, ebiederm, luto, carlos, dhowells, viro,
simo, Eric Paris, Serge Hallyn
In-Reply-To: <8e617ab568df28a66dfbe3284452de186b42fb0f.1533065887.git.rgb@redhat.com>
On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> The audit-related parameters in struct task_struct should ideally be
> collected together and accessed through a standard audit API.
>
> Collect the existing loginuid, sessionid and audit_context together in a
> new struct audit_task_info called "audit" in struct task_struct.
>
> Use kmem_cache to manage this pool of memory.
> Un-inline audit_free() to be able to always recover that memory.
>
> See: https://github.com/linux-audit/audit-kernel/issues/81
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 34 ++++++++++++++++++++++++----------
> include/linux/sched.h | 5 +----
> init/init_task.c | 3 +--
> init/main.c | 2 ++
> kernel/auditsc.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
> kernel/fork.c | 4 +++-
> 6 files changed, 73 insertions(+), 26 deletions(-)
...
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 9334fbe..8964332 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -219,8 +219,15 @@ static inline void audit_log_task_info(struct audit_buffer *ab,
>
> /* These are defined in auditsc.c */
> /* Public API */
> +struct audit_task_info {
> + kuid_t loginuid;
> + unsigned int sessionid;
> + struct audit_context *ctx;
> +};
...
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 87bf02d..e117272 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -873,10 +872,8 @@ struct task_struct {
>
> struct callback_head *task_works;
>
> - struct audit_context *audit_context;
> #ifdef CONFIG_AUDITSYSCALL
> - kuid_t loginuid;
> - unsigned int sessionid;
> + struct audit_task_info *audit;
> #endif
> struct seccomp seccomp;
Prior to this patch audit_context was available regardless of
CONFIG_AUDITSYSCALL, after this patch the corresponding audit_context
is only available when CONFIG_AUDITSYSCALL is defined.
> diff --git a/init/main.c b/init/main.c
> index 3b4ada1..6aba171 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -92,6 +92,7 @@
> #include <linux/rodata_test.h>
> #include <linux/jump_label.h>
> #include <linux/mem_encrypt.h>
> +#include <linux/audit.h>
>
> #include <asm/io.h>
> #include <asm/bugs.h>
> @@ -721,6 +722,7 @@ asmlinkage __visible void __init start_kernel(void)
> nsfs_init();
> cpuset_init();
> cgroup_init();
> + audit_task_init();
> taskstats_init_early();
> delayacct_init();
It seems like we would need either init_struct_audit or
audit_task_init(), but not both, yes?
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index fb20746..88779a7 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -841,7 +841,7 @@ static inline struct audit_context *audit_take_context(struct task_struct *tsk,
> int return_valid,
> long return_code)
> {
> - struct audit_context *context = tsk->audit_context;
> + struct audit_context *context = tsk->audit->ctx;
>
> if (!context)
> return NULL;
> @@ -926,6 +926,15 @@ static inline struct audit_context *audit_alloc_context(enum audit_state state)
> return context;
> }
>
> +static struct kmem_cache *audit_task_cache;
> +
> +void __init audit_task_init(void)
> +{
> + audit_task_cache = kmem_cache_create("audit_task",
> + sizeof(struct audit_task_info),
> + 0, SLAB_PANIC, NULL);
> +}
This is somewhat related to the CONFIG_AUDITSYSCALL comment above, but
since the audit_task_info contains generic audit state (not just
syscall related state), it seems like this, and the audit_task_info
accessors/helpers, should live in kernel/audit.c.
There are probably a few other things that should move to
kernel/audit.c too, e.g. audit_alloc(). Have you verified that this
builds/runs correctly on architectures that define CONFIG_AUDIT but
not CONFIG_AUDITSYSCALL?
> /**
> * audit_alloc - allocate an audit context block for a task
> * @tsk: task
> @@ -940,17 +949,28 @@ int audit_alloc(struct task_struct *tsk)
> struct audit_context *context;
> enum audit_state state;
> char *key = NULL;
> + struct audit_task_info *info;
> +
> + info = kmem_cache_zalloc(audit_task_cache, GFP_KERNEL);
> + if (!info)
> + return -ENOMEM;
> + info->loginuid = audit_get_loginuid(current);
> + info->sessionid = audit_get_sessionid(current);
> + tsk->audit = info;
>
> if (likely(!audit_ever_enabled))
> return 0; /* Return if not auditing. */
I don't view this as necessary for initial acceptance, and
synchronization/locking might render this undesirable, but it would be
curious to see if we could do something clever with refcnts and
copy-on-write to minimize the number of kmem_cache objects in use in
the !audit_ever_enabled (and possibly the AUDIT_DISABLED) case.
> state = audit_filter_task(tsk, &key);
> if (state == AUDIT_DISABLED) {
> + audit_set_context(tsk, NULL);
It's already NULL, isn't it?
> clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
> return 0;
> }
>
> if (!(context = audit_alloc_context(state))) {
> + tsk->audit = NULL;
> + kmem_cache_free(audit_task_cache, info);
> kfree(key);
> audit_log_lost("out of memory in audit_alloc");
> return -ENOMEM;
> @@ -962,6 +982,12 @@ int audit_alloc(struct task_struct *tsk)
> return 0;
> }
>
> +struct audit_task_info init_struct_audit = {
> + .loginuid = INVALID_UID,
> + .sessionid = AUDIT_SID_UNSET,
> + .ctx = NULL,
> +};
> +
> static inline void audit_free_context(struct audit_context *context)
> {
> audit_free_names(context);
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 03/10] audit: log container info of syscalls
From: Paul Moore @ 2018-10-19 23:16 UTC (permalink / raw)
To: rgb
Cc: containers, linux-api, linux-audit, linux-fsdevel, linux-kernel,
netdev, netfilter-devel, ebiederm, luto, carlos, dhowells, viro,
simo, Eric Paris, Serge Hallyn
In-Reply-To: <34017c395d03a213d6b0d49b9964429bd32b283d.1533065887.git.rgb@redhat.com>
On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> Create a new audit record AUDIT_CONTAINER to document the audit
> container identifier of a process if it is present.
>
> Called from audit_log_exit(), syscalls are covered.
>
> A sample raw event:
> type=SYSCALL msg=audit(1519924845.499:257): arch=c000003e syscall=257 success=yes exit=3 a0=ffffff9c a1=56374e1cef30 a2=241 a3=1b6 items=2 ppid=606 pid=635 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=3 comm="bash" exe="/usr/bin/bash" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key="tmpcontainerid"
> type=CWD msg=audit(1519924845.499:257): cwd="/root"
> type=PATH msg=audit(1519924845.499:257): item=0 name="/tmp/" inode=13863 dev=00:27 mode=041777 ouid=0 ogid=0 rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype= PARENT cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> type=PATH msg=audit(1519924845.499:257): item=1 name="/tmp/tmpcontainerid" inode=17729 dev=00:27 mode=0100644 ouid=0 ogid=0 rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=0000000000000000 cap_fi=0000000000000000 cap_fe=0 cap_fver=0
> type=PROCTITLE msg=audit(1519924845.499:257): proctitle=62617368002D6300736C65657020313B206563686F2074657374203E202F746D702F746D70636F6E7461696E65726964
> type=CONTAINER msg=audit(1519924845.499:257): op=task contid=123458
>
> See: https://github.com/linux-audit/audit-kernel/issues/90
> See: https://github.com/linux-audit/audit-userspace/issues/51
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> Acked-by: Steve Grubb <sgrubb@redhat.com>
> ---
> include/linux/audit.h | 7 +++++++
> include/uapi/linux/audit.h | 1 +
> kernel/audit.c | 24 ++++++++++++++++++++++++
> kernel/auditsc.c | 3 +++
> 4 files changed, 35 insertions(+)
...
> @@ -2045,6 +2045,30 @@ void audit_log_session_info(struct audit_buffer *ab)
> audit_log_format(ab, " auid=%u ses=%u", auid, sessionid);
> }
>
> +/*
> + * audit_log_contid - report container info
> + * @tsk: task to be recorded
> + * @context: task or local context for record
> + * @op: contid string description
> + */
> +int audit_log_contid(struct task_struct *tsk,
> + struct audit_context *context, char *op)
> +{
> + struct audit_buffer *ab;
> +
> + if (!audit_contid_set(tsk))
> + return 0;
> + /* Generate AUDIT_CONTAINER record with container ID */
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> + if (!ab)
> + return -ENOMEM;
> + audit_log_format(ab, "op=%s contid=%llu",
> + op, audit_get_contid(tsk));
> + audit_log_end(ab);
> + return 0;
> +}
> +EXPORT_SYMBOL(audit_log_contid);
As discussed in the previous iteration of the patch, I prefer
AUDIT_CONTAINER_ID here over AUDIT_CONTAINER. If you feel strongly
about keeping it as-is with AUDIT_CONTAINER I suppose I could live
with that, but it is isn't my first choice.
However, I do care about the "op" field in this record. It just
doesn't make any sense; the way you are using it it is more of a
context field than an operations field, and even then why is the
context important from a logging and/or security perspective? Drop it
please.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 04/10] audit: add containerid support for ptrace and signals
From: Paul Moore @ 2018-10-19 23:16 UTC (permalink / raw)
To: rgb
Cc: containers, linux-api, linux-audit, linux-fsdevel, linux-kernel,
netdev, netfilter-devel, ebiederm, luto, carlos, dhowells, viro,
simo, Eric Paris, Serge Hallyn
In-Reply-To: <71c7761b5e0289fee7d82ddcc723d39804826e53.1533065887.git.rgb@redhat.com>
On Tue, Jul 31, 2018 at 4:11 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Add audit container identifier support to ptrace and signals. In
> particular, the "op" field provides a way to label the auxiliary record
> to which it is associated.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> ---
> include/linux/audit.h | 11 +++++------
> kernel/audit.c | 13 +++++++------
> kernel/audit.h | 2 ++
> kernel/auditsc.c | 21 ++++++++++++++++-----
> 4 files changed, 30 insertions(+), 17 deletions(-)
...
> @@ -155,9 +156,8 @@ extern void audit_log_key(struct audit_buffer *ab,
> extern int audit_log_task_context(struct audit_buffer *ab);
> extern void audit_log_task_info(struct audit_buffer *ab,
> struct task_struct *tsk);
> -extern int audit_log_contid(struct task_struct *tsk,
> - struct audit_context *context,
> - char *op);
> +extern int audit_log_contid(struct audit_context *context,
> + char *op, u64 contid);
>
> extern int audit_update_lsm_rules(void);
...
> @@ -2047,23 +2049,22 @@ void audit_log_session_info(struct audit_buffer *ab)
>
> /*
> * audit_log_contid - report container info
> - * @tsk: task to be recorded
> * @context: task or local context for record
> * @op: contid string description
> + * @contid: container ID to report
> */
> -int audit_log_contid(struct task_struct *tsk,
> - struct audit_context *context, char *op)
> +int audit_log_contid(struct audit_context *context,
> + char *op, u64 contid)
> {
> struct audit_buffer *ab;
>
> - if (!audit_contid_set(tsk))
> + if (!audit_contid_valid(contid))
> return 0;
> /* Generate AUDIT_CONTAINER record with container ID */
> ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER);
> if (!ab)
> return -ENOMEM;
> - audit_log_format(ab, "op=%s contid=%llu",
> - op, audit_get_contid(tsk));
> + audit_log_format(ab, "op=%s contid=%llu", op, contid);
> audit_log_end(ab);
> return 0;
> }
My previous comments still apply: these audit_log_contid() changes
should be done earlier in the patchset when you first define
audit_log_contid().
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 05/10] audit: add support for non-syscall auxiliary records
From: Paul Moore @ 2018-10-19 23:17 UTC (permalink / raw)
To: rgb
Cc: containers, linux-api, linux-audit, linux-fsdevel, linux-kernel,
netdev, netfilter-devel, ebiederm, luto, carlos, dhowells, viro,
simo, Eric Paris, Serge Hallyn
In-Reply-To: <2827530000d6b4972d446b0226adab153ff3b5c5.1533065887.git.rgb@redhat.com>
On Sun, Aug 5, 2018 at 4:33 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> Standalone audit records have the timestamp and serial number generated
> on the fly and as such are unique, making them standalone. This new
> function audit_alloc_local() generates a local audit context that will
> be used only for a standalone record and its auxiliary record(s). The
> context is discarded immediately after the local associated records are
> produced.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> ---
> include/linux/audit.h | 8 ++++++++
> kernel/audit.h | 1 +
> kernel/auditsc.c | 33 ++++++++++++++++++++++++++++-----
> 3 files changed, 37 insertions(+), 5 deletions(-)
I'm not in love with the local flag, and the whole local context in
general, but that's a larger discussion and not something I want to
force on this patchset; we can fix it later.
I think this patch looks fine, but it seems a bit odd standalone; it's
almost always better to include new capabilities/functions in the same
patch as the user. Since the only user is the networking bits, it
might make more sense to fold this patch into that one.
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 4f514ed..1f340ad 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -234,7 +234,9 @@ struct audit_task_info {
> extern struct audit_task_info init_struct_audit;
> extern void __init audit_task_init(void);
> extern int audit_alloc(struct task_struct *task);
> +extern struct audit_context *audit_alloc_local(gfp_t gfpflags);
> extern void audit_free(struct task_struct *task);
> +extern void audit_free_context(struct audit_context *context);
> extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
> unsigned long a2, unsigned long a3);
> extern void __audit_syscall_exit(int ret_success, long ret_value);
> @@ -495,6 +497,12 @@ static inline int audit_alloc(struct task_struct *task)
> {
> return 0;
> }
> +static inline struct audit_context *audit_alloc_local(gfp_t gfpflags)
> +{
> + return NULL;
> +}
> +static inline void audit_free_context(struct audit_context *context)
> +{ }
> static inline void audit_free(struct task_struct *task)
> { }
> static inline void audit_syscall_entry(int major, unsigned long a0,
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 1cf1c35..a6d00a5 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -110,6 +110,7 @@ struct audit_proctitle {
> struct audit_context {
> int dummy; /* must be the first element */
> int in_syscall; /* 1 if task is in a syscall */
> + bool local; /* local context needed */
> enum audit_state state, current_state;
> unsigned int serial; /* serial number for record */
> int major; /* syscall number */
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index cdb24cf..7627f21 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -913,11 +913,13 @@ static inline void audit_free_aux(struct audit_context *context)
> }
> }
>
> -static inline struct audit_context *audit_alloc_context(enum audit_state state)
> +static inline struct audit_context *audit_alloc_context(enum audit_state state,
> + gfp_t gfpflags)
> {
> struct audit_context *context;
>
> - context = kzalloc(sizeof(*context), GFP_KERNEL);
> + /* We can be called in atomic context via audit_tg() */
> + context = kzalloc(sizeof(*context), gfpflags);
> if (!context)
> return NULL;
> context->state = state;
> @@ -970,7 +972,8 @@ int audit_alloc(struct task_struct *tsk)
> return 0;
> }
>
> - if (!(context = audit_alloc_context(state))) {
> + context = audit_alloc_context(state, GFP_KERNEL);
> + if (!(context)) {
> tsk->audit = NULL;
> kmem_cache_free(audit_task_cache, info);
> kfree(key);
> @@ -991,8 +994,27 @@ struct audit_task_info init_struct_audit = {
> .ctx = NULL,
> };
>
> -static inline void audit_free_context(struct audit_context *context)
> +struct audit_context *audit_alloc_local(gfp_t gfpflags)
> {
> + struct audit_context *context;
> +
> + if (!audit_ever_enabled)
> + return NULL; /* Return if not auditing. */
> +
> + context = audit_alloc_context(AUDIT_RECORD_CONTEXT, gfpflags);
> + if (!context)
> + return NULL;
> + context->serial = audit_serial();
> + context->ctime = current_kernel_time64();
> + context->local = true;
> + return context;
> +}
> +EXPORT_SYMBOL(audit_alloc_local);
> +
> +void audit_free_context(struct audit_context *context)
> +{
> + if (!context)
> + return;
> audit_free_names(context);
> unroll_tree_refs(context, NULL, 0);
> free_tree_refs(context);
> @@ -1002,6 +1024,7 @@ static inline void audit_free_context(struct audit_context *context)
> audit_proctitle_free(context);
> kfree(context);
> }
> +EXPORT_SYMBOL(audit_free_context);
>
> static int audit_log_pid_context(struct audit_context *context, pid_t pid,
> kuid_t auid, kuid_t uid, unsigned int sessionid,
> @@ -2024,7 +2047,7 @@ void __audit_inode_child(struct inode *parent,
> int auditsc_get_stamp(struct audit_context *ctx,
> struct timespec64 *t, unsigned int *serial)
> {
> - if (!ctx->in_syscall)
> + if (!ctx->in_syscall && !ctx->local)
> return 0;
> if (!ctx->serial)
> ctx->serial = audit_serial();
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 06/10] audit: add containerid support for tty_audit
From: Paul Moore @ 2018-10-19 23:17 UTC (permalink / raw)
To: rgb
Cc: containers, linux-api, linux-audit, linux-fsdevel, linux-kernel,
netdev, netfilter-devel, ebiederm, luto, carlos, dhowells, viro,
simo, Eric Paris, Serge Hallyn
In-Reply-To: <e41cb6760a8183c0955c378fce7b500819f9838f.1533065887.git.rgb@redhat.com>
On Sun, Aug 5, 2018 at 4:33 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> Add audit container identifier auxiliary record to tty logging rule
> event standalone records.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> ---
> drivers/tty/tty_audit.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/tty_audit.c b/drivers/tty/tty_audit.c
> index 50f567b..3e21477 100644
> --- a/drivers/tty/tty_audit.c
> +++ b/drivers/tty/tty_audit.c
> @@ -66,8 +66,9 @@ static void tty_audit_log(const char *description, dev_t dev,
> uid_t uid = from_kuid(&init_user_ns, task_uid(tsk));
> uid_t loginuid = from_kuid(&init_user_ns, audit_get_loginuid(tsk));
> unsigned int sessionid = audit_get_sessionid(tsk);
> + struct audit_context *context = audit_alloc_local(GFP_KERNEL);
>
> - ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_TTY);
> + ab = audit_log_start(context, GFP_KERNEL, AUDIT_TTY);
> if (ab) {
> char name[sizeof(tsk->comm)];
>
> @@ -80,6 +81,8 @@ static void tty_audit_log(const char *description, dev_t dev,
> audit_log_n_hex(ab, data, size);
> audit_log_end(ab);
> }
> + audit_log_contid(context, "tty", audit_get_contid(tsk));
> + audit_free_context(context);
> }
Since I never polished up my task_struct/current fix patch enough to
get it past RFC status during this development window (new job, stolen
laptop, etc.) *and* it looks like you are going to need at least one
more respin of this patchset, go ahead and fix this patch to use
current instead of generating a local context. I'll deal with the
merge fallout if/when it happens.
Local contexts are a last resort. If you ever find yourself writing
code that generates a local context, you should first be 100% certain
that the event is not the the result of a process initiated action (in
which case it should take from the task's context).
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 08/10] audit: add support for containerid to network namespaces
From: Paul Moore @ 2018-10-19 23:18 UTC (permalink / raw)
To: rgb
Cc: containers, linux-api, linux-audit, linux-fsdevel, linux-kernel,
netdev, netfilter-devel, ebiederm, luto, carlos, dhowells, viro,
simo, Eric Paris, Serge Hallyn
In-Reply-To: <5a2b4aadf6994f622bc1ad27a8a6889c7e61edff.1533065887.git.rgb@redhat.com>
On Tue, Jul 31, 2018 at 4:12 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Audit events could happen in a network namespace outside of a task
> context due to packets received from the net that trigger an auditing
> rule prior to being associated with a running task. The network
> namespace could in use by multiple containers by association to the
> tasks in that network namespace. We still want a way to attribute
> these events to any potential containers. Keep a list per network
> namespace to track these audit container identifiiers.
>
> Add/increment the audit container identifier on:
> - initial setting of the audit container identifier via /proc
> - clone/fork call that inherits an audit container identifier
> - unshare call that inherits an audit container identifier
> - setns call that inherits an audit container identifier
> Delete/decrement the audit container identifier on:
> - an inherited audit container identifier dropped when child set
> - process exit
> - unshare call that drops a net namespace
> - setns call that drops a net namespace
>
> See: https://github.com/linux-audit/audit-kernel/issues/92
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 17 ++++++++++
> kernel/audit.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++++
> kernel/auditsc.c | 8 ++++-
> kernel/nsproxy.c | 4 +++
> 4 files changed, 114 insertions(+), 1 deletion(-)
...
> @@ -308,6 +312,86 @@ static struct sock *audit_get_sk(const struct net *net)
> return aunet->sk;
> }
>
> +/**
> + * audit_get_netns_contid_list - Return the audit container ID list for the given network namespace
> + * @net: the destination network namespace
> + *
> + * Description:
> + * Returns the list pointer if valid, NULL otherwise. The caller must ensure
> + * that a reference is held for the network namespace while the sock is in use.
> + */
> +struct list_head *audit_get_netns_contid_list(const struct net *net)
> +{
> + struct audit_net *aunet = net_generic(net, audit_net_id);
> +
> + return &aunet->contid_list;
> +}
> +
> +spinlock_t *audit_get_netns_contid_list_lock(const struct net *net)
> +{
> + struct audit_net *aunet = net_generic(net, audit_net_id);
> +
> + return &aunet->contid_list_lock;
> +}
Instead of returning the spinlock, just do away with the
audit_get_ns_contid_list_lock() function and create two separate lock
and unlock functions that basically do the net_generic() and spinlock
operations together, for example:
static int audit_netns_contid_lock(const struct net *net)
{
aunet = net_generic(net, audit_net_id);
if (!aunet)
return -whatever;
spin_lock(aunet->lock);
return 0;
}
> +void audit_netns_contid_add(struct net *net, u64 contid)
> +{
> + spinlock_t *lock = audit_get_netns_contid_list_lock(net);
> + struct list_head *contid_list = audit_get_netns_contid_list(net);
> + struct audit_contid *cont;
> +
> + if (!audit_contid_valid(contid))
> + return;
> + spin_lock(lock);
> + if (!list_empty(contid_list))
> + list_for_each_entry(cont, contid_list, list)
> + if (cont->id == contid) {
> + refcount_inc(&cont->refcount);
> + goto out;
> + }
> + cont = kmalloc(sizeof(struct audit_contid), GFP_KERNEL);
> + if (cont) {
> + INIT_LIST_HEAD(&cont->list);
> + cont->id = contid;
> + refcount_set(&cont->refcount, 1);
> + list_add(&cont->list, contid_list);
> + }
> +out:
> + spin_unlock(lock);
> +}
> +
> +void audit_netns_contid_del(struct net *net, u64 contid)
> +{
> + spinlock_t *lock = audit_get_netns_contid_list_lock(net);
> + struct list_head *contid_list = audit_get_netns_contid_list(net);
> + struct audit_contid *cont = NULL;
> +
> + if (!audit_contid_valid(contid))
> + return;
> + spin_lock(lock);
> + if (!list_empty(contid_list))
> + list_for_each_entry(cont, contid_list, list)
> + if (cont->id == contid) {
> + list_del(&cont->list);
> + if (refcount_dec_and_test(&cont->refcount))
> + kfree(cont);
> + break;
> + }
> + spin_unlock(lock);
> +}
> +
> +void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> +{
> + u64 contid = audit_get_contid(p);
> + struct nsproxy *new = p->nsproxy;
> +
> + if (!audit_contid_valid(contid))
> + return;
> + audit_netns_contid_del(ns->net_ns, contid);
> + if (new)
> + audit_netns_contid_add(new->net_ns, contid);
> +}
> +
> void audit_panic(const char *message)
> {
> switch (audit_failure) {
> @@ -1547,6 +1631,8 @@ static int __net_init audit_net_init(struct net *net)
> return -ENOMEM;
> }
> aunet->sk->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
> + INIT_LIST_HEAD(&aunet->contid_list);
> + spin_lock_init(&aunet->contid_list_lock);
>
> return 0;
> }
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 610c6869..fdf3f68 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -75,6 +75,7 @@
> #include <linux/uaccess.h>
> #include <linux/fsnotify_backend.h>
> #include <uapi/linux/limits.h>
> +#include <net/net_namespace.h>
>
> #include "audit.h"
>
> @@ -2165,6 +2166,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> uid_t uid;
> struct tty_struct *tty;
> char comm[sizeof(current->comm)];
> + struct net *net = task->nsproxy->net_ns;
>
> task_lock(task);
> /* Can't set if audit disabled */
> @@ -2186,8 +2188,12 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> else if (!(thread_group_leader(task) && thread_group_empty(task)))
> rc = -EALREADY;
> read_unlock(&tasklist_lock);
> - if (!rc)
> + if (!rc) {
> + if (audit_contid_valid(oldcontid))
> + audit_netns_contid_del(net, oldcontid);
> task->audit->contid = contid;
> + audit_netns_contid_add(net, contid);
> + }
> task_unlock(task);
>
> if (!audit_enabled)
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index f6c5d33..718b120 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -27,6 +27,7 @@
> #include <linux/syscalls.h>
> #include <linux/cgroup.h>
> #include <linux/perf_event.h>
> +#include <linux/audit.h>
>
> static struct kmem_cache *nsproxy_cachep;
>
> @@ -140,6 +141,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> struct nsproxy *old_ns = tsk->nsproxy;
> struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns);
> struct nsproxy *new_ns;
> + u64 contid = audit_get_contid(tsk);
>
> if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
> CLONE_NEWPID | CLONE_NEWNET |
> @@ -167,6 +169,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> return PTR_ERR(new_ns);
>
> tsk->nsproxy = new_ns;
> + audit_netns_contid_add(new_ns->net_ns, contid);
> return 0;
> }
>
> @@ -224,6 +227,7 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
> ns = p->nsproxy;
> p->nsproxy = new;
> task_unlock(p);
> + audit_switch_task_namespaces(ns, p);
>
> if (ns && atomic_dec_and_test(&ns->count))
> free_nsproxy(ns);
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 09/10] audit: NETFILTER_PKT: record each container ID associated with a netNS
From: Paul Moore @ 2018-10-19 23:18 UTC (permalink / raw)
To: rgb
Cc: containers, linux-api, linux-audit, linux-fsdevel, linux-kernel,
netdev, netfilter-devel, ebiederm, luto, carlos, dhowells, viro,
simo, Eric Paris, Serge Hallyn
In-Reply-To: <3f5edfb0d530d7f0061fe11b817b315b350b9d86.1533065887.git.rgb@redhat.com>
On Sun, Aug 5, 2018 at 4:33 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> Add audit container identifier auxiliary record(s) to NETFILTER_PKT
> event standalone records. Iterate through all potential audit container
> identifiers associated with a network namespace.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 5 +++++
> kernel/audit.c | 26 ++++++++++++++++++++++++++
> net/netfilter/xt_AUDIT.c | 12 ++++++++++--
> 3 files changed, 41 insertions(+), 2 deletions(-)
...
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 9a02095..8755f4d 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -169,6 +169,8 @@ extern int audit_log_contid(struct audit_context *context,
> extern void audit_netns_contid_add(struct net *net, u64 contid);
> extern void audit_netns_contid_del(struct net *net, u64 contid);
> extern void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p);
> +extern void audit_log_netns_contid_list(struct net *net,
> + struct audit_context *context);
>
> extern int audit_update_lsm_rules(void);
>
> @@ -228,6 +230,9 @@ static inline void audit_netns_contid_del(struct net *net, u64 contid)
> { }
> static inline void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> { }
> +static inline void audit_log_netns_contid_list(struct net *net,
> + struct audit_context *context)
> +{ }
>
> #define audit_enabled AUDIT_OFF
> #endif /* CONFIG_AUDIT */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index c5fed3b..b23711c 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -392,6 +392,32 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> audit_netns_contid_add(new->net_ns, contid);
> }
>
> +void audit_log_netns_contid_list(struct net *net, struct audit_context *context)
> +{
> + spinlock_t *lock = audit_get_netns_contid_list_lock(net);
> + struct audit_buffer *ab;
> + struct audit_contid *cont;
> + bool first = true;
> +
> + /* Generate AUDIT_CONTAINER record with container ID CSV list */
> + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_CONTAINER);
> + if (!ab) {
> + audit_log_lost("out of memory in audit_log_netns_contid_list");
> + return;
> + }
> + audit_log_format(ab, "contid=");
> + spin_lock(lock);
> + list_for_each_entry(cont, audit_get_netns_contid_list(net), list) {
> + if (!first)
> + audit_log_format(ab, ",");
> + audit_log_format(ab, "%llu", cont->id);
> + first = false;
> + }
> + spin_unlock(lock);
This is looking like potentially a lot of work to be doing under a
spinlock, not to mention a single spinlock that is shared across CPUs.
Considering that I expect changes to the list to be somewhat
infrequent, this might be a good candidate for a RCU based locking
scheme.
> + audit_log_end(ab);
> +}
> +EXPORT_SYMBOL(audit_log_netns_contid_list);
>
> void audit_panic(const char *message)
> {
> switch (audit_failure) {
> diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> index af883f1..44fac3f 100644
> --- a/net/netfilter/xt_AUDIT.c
> +++ b/net/netfilter/xt_AUDIT.c
> @@ -71,10 +71,13 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
> {
> struct audit_buffer *ab;
> int fam = -1;
> + struct audit_context *context;
> + struct net *net;
>
> if (audit_enabled == AUDIT_OFF)
> - goto errout;
> - ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> + goto out;
> + context = audit_alloc_local(GFP_ATOMIC);
> + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> if (ab == NULL)
> goto errout;
>
> @@ -104,7 +107,12 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
>
> audit_log_end(ab);
>
> + net = xt_net(par);
> + audit_log_netns_contid_list(net, context);
> +
> errout:
> + audit_free_context(context);
> +out:
> return XT_CONTINUE;
> }
>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v2 6/5] statx: add STATX_RESULT_MASK flag
From: David Howells @ 2018-10-19 23:50 UTC (permalink / raw)
To: Miklos Szeredi
Cc: dhowells, Miklos Szeredi, linux-fsdevel, linux-kernel, Linux API,
Michael Kerrisk, Andreas Dilger, Florian Weimer, Amir Goldstein
In-Reply-To: <CAJfpegtPv4+QxzcqMAGwTt7QALjBaxQtt2Qet98pdmuT1MRUeA@mail.gmail.com>
Miklos Szeredi <miklos@szeredi.hu> wrote:
> This is very much about the basic statx fields. I.e. what does
> result_mask == STATX_BASIC_STATS means?
>
> a) this is a legacy stat() implementation that doesn't fill in the
> result_mask properly
>
> b) this is an up-todate stat() implementation that does fill the
> result_mask properly and all fields are valid
>
> There's no way to tell which one it is, and no way to request that
> filesystem try b) even if it's more costly.
Okay, I think I see what you're getting at. We need to be able to tell the
user that we don't actually know what's good and what's not. I guess this
doesn't only apply to fuse, but could also apply to nfs potentially as nfs
doesn't necessarily know what the server is capable of and where it's making
stuff up (imagine an NFS server backing both an ext4 and a fat filesystem).
I would be tempted to represent this with an attribute flag instead, say:
STATX_ATTR_UNCERTAIN_VALUES
Your attributes are actually in one of at least three states, not two:
unsupported, definite and uncertain. Definite things might include such as
STATX_TYPE - after all, if you don't know what type a file is, you can't
really use it.
If someone really needs to know, it might be worth deferring further
information to fsinfo().
David
^ permalink raw reply
* Re: [PATCH v2 6/5] statx: add STATX_RESULT_MASK flag
From: Andreas Dilger @ 2018-10-20 5:55 UTC (permalink / raw)
To: Miklos Szeredi
Cc: David Howells, Miklos Szeredi, Linux FS-devel Mailing List,
linux-kernel, Linux API, Michael Kerrisk, Florian Weimer,
Amir Goldstein
In-Reply-To: <CAJfpegtPv4+QxzcqMAGwTt7QALjBaxQtt2Qet98pdmuT1MRUeA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]
On Oct 19, 2018, at 11:42 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>>> +#define STATX_RESULT_MASK STATX__RESERVED
>>
>> Please don't use that bit.
>
> Using it internally is perfectly harmless. If we'll need to extend
> statx in the future and make use of this flag externally, then we can
> easily move the internal flag somewhere else (e.g. extend request_mask
> to 64bit, which we'll probably need to do anyway in that case).
I was thinking about this - what is the point of returning an error
if STATX__RESERVED is set? If this is used to indicate the presence
of e.g. stx_mask2, then newer applications trying to request any of the
flags encoded into stx_mask2 will get an error, rather than the expected
behaviour of "ignore flags you don't understand, and don't set them in
the return stx_mask".
Essentially, this will make STATX__RESERVED useless in the future, since
no application will be able to use it without getting an error if they
are running on an old kernel.
Cheers, Andreas
[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]
^ permalink raw reply
* Re: [PATCH v2 5/5] nfs: don't clear STATX_ATIME from result_mask
From: Trond Myklebust @ 2018-10-20 17:46 UTC (permalink / raw)
To: miklos@szeredi.hu
Cc: linux-kernel@vger.kernel.org, amir73il@gmail.com,
mszeredi@redhat.com, adilger@dilger.ca, linux-api@vger.kernel.org,
dhowells@redhat.com, fw@deneb.enyo.de, mtk.manpages@gmail.com,
linux-fsdevel@vger.kernel.org
In-Reply-To: <CAJfpeguFe=KcahJmZire52TPppMk8amiTJFiBZuv=97bRbCj8g@mail.gmail.com>
On Fri, 2018-10-19 at 22:48 +0200, Miklos Szeredi wrote:
> On Fri, Oct 19, 2018 at 8:14 PM, Trond Myklebust
> <trondmy@hammerspace.com> wrote:
> > On Fri, 2018-10-19 at 19:46 +0200, Miklos Szeredi wrote:
> > > How is it then that only STATX_ATIME is cleared and not the other
> > > fields?
> >
> > It isn't just the atime. We can also fail to revalidate the ctime
> > and
> > mtime if they are not being requested by the user.
> >
> > >
> > > Note: junk != stale. The statx definition doesn't talk about the
> > > fields being up-to-date, except for AT_STATX_FORCE_SYNC, so stale
> > > attributes are okay, and do not warrant clearing the result_mask.
> > >
> >
> > I disagree. stale == junk here, because the default of
> > AT_STATX_SYNC_AS_STAT is described by the manpage as "Do whatever
> > stat(2) does." which this is not.
>
> Ah, you are talking about this:
>
> /* Is the user requesting attributes that might need
> revalidation? */
> if (!(request_mask &
> (STATX_MODE|STATX_NLINK|STATX_ATIME|STATX_CTIME|
> STATX_MTIME|STATX_UID|STATX_GID|
> STATX_SIZE|STATX_BLOCKS)))
> goto out_no_update;
>
> Well, if this is triggered for statx(..., STATX_ATIME,
> AT_STATX_SYNC_AS_STAT) and MNT_NOATIME, then yes, result will be
> junk.
> Which means that the code is wrong, it shouldn't do that.
The problem is that vfs_getattr_nosec() populates stat->result_mask
with a default of STATX_BASIC_STATS, which makes no sense unless you
assume that the user will always ask for a superset of
STATX_BASIC_STATS (or you assume that those attributes never need
revalidation, which is obviously braindead).
> Otherwise (if something other than STATX_ATIME or STATX_INO or
> STATX_TYPE is given as well) it *will* do the same thing as what
> stat(2) does, so in that case STATX_ATIME should not be cleared (yet
> it is cleared).
As far as I'm concerned, we can definitely get rid of the
/*
* We may force a getattr if the user cares about atime.
*
* Note that we only have to check the vfsmount flags here:
* - NFS always sets S_NOATIME by so checking it would give a
* bogus result
* - NFS never sets SB_NOATIME or SB_NODIRATIME so there is
* no point in checking those.
*/
if ((path->mnt->mnt_flags & MNT_NOATIME) ||
((path->mnt->mnt_flags & MNT_NODIRATIME) && S_ISDIR(inode->i_mode)))
request_mask &= ~STATX_ATIME;
however the rest needs to stay, or there is no way we can use statx()
to allow optimised retrieval of only those attributes that your
application cares about.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com
^ permalink raw reply
* Re: [RFC 00/20] ns: Introduce Time Namespace
From: Andrei Vagin @ 2018-10-21 1:41 UTC (permalink / raw)
To: Eric W. Biederman, Thomas Gleixner
Cc: linux-kselftest@vger.kernel.org, Andrei Vagin, Dmitry Safonov,
linux-api@vger.kernel.org, Jeff Dike, x86@kernel.org,
Dmitry Safonov, linux-kernel@vger.kernel.org, Oleg Nesterov,
criu@openvz.org, Ingo Molnar, Alexey Dobriyan, Andy Lutomirski,
H. Peter Anvin, Cyrill Gorcunov, Christian Brauner,
Pavel Emelianov, Shuah Khan, containers
In-Reply-To: <87mus1ftb9.fsf@xmission.com>
On Fri, Sep 28, 2018 at 07:03:22PM +0200, Eric W. Biederman wrote:
> Thomas Gleixner <tglx@linutronix.de> writes:
>
> > On Wed, 26 Sep 2018, Eric W. Biederman wrote:
> >> Reading the code the calling sequence there is:
> >> tick_sched_do_timer
> >> tick_do_update_jiffies64
> >> update_wall_time
> >> timekeeping_advance
> >> timekeepging_update
> >>
> >> If I read that properly under the right nohz circumstances that update
> >> can be delayed indefinitely.
> >>
> >> So I think we could prototype a time namespace that was per
> >> timekeeping_update and just had update_wall_time iterate through
> >> all of the time namespaces.
> >
> > Please don't go there. timekeeping_update() is already heavy and walking
> > through a gazillion of namespaces will just make it horrible,
> >
> >> I don't think the naive version would scale to very many time
> >> namespaces.
> >
> > :)
> >
> >> At the same time using the techniques from the nohz work and a little
> >> smarts I expect we could get the code to scale.
> >
> > You'd need to invoke the update when the namespace is switched in and
> > hasn't been updated since the last tick happened. That might be doable, but
> > you also need to take the wraparound constraints of the underlying
> > clocksources into account, which again can cause walking all name spaces
> > when they are all idle long enough.
>
> The wrap around constraints being how long before the time sources wrap
> around so you have to read them once per wrap around? I have not dug
> deeply enough into the code to see that yet.
>
> > From there it becomes hairy, because it's not only timekeeping,
> > i.e. reading time, this is also affecting all timers which are armed from a
> > namespace.
> >
> > That gets really ugly because when you do settimeofday() or adjtimex() for
> > a particular namespace, then you have to search for all armed timers of
> > that namespace and adjust them.
> >
> > The original posix timer code had the same issue because it mapped the
> > clock realtime timers to the timer wheel so any setting of the clock caused
> > a full walk of all armed timers, disarming, adjusting and requeing
> > them. That's horrible not only performance wise, it's also a locking
> > nightmare of all sorts.
> >
> > Add time skew via NTP/PTP into the picture and you might have to adjust
> > timers as well, because you need to guarantee that they are not expiring
> > early.
> >
> > I haven't looked through Dimitry's patches yet, but I don't see how this
> > can work at all without introducing subtle issues all over the place.
>
> Then it sounds like this will take some more digging.
>
> Please pardon me for thinking out load.
>
> There are one or more time sources that we use to compute the time
> and for each time source we have a conversion from ticks of the
> time source to nanoseconds.
>
> Each time source needs to be sampled at least once per wrap-around
> and something incremented so that we don't loose time when looking
> at that time source.
>
> There are several clocks presented to userspace and they all share the
> same length of second and are all fundamentally offsets from
> CLOCK_MONOTONIC.
>
> I see two fundamental driving cases for a time namespace.
> 1) Migration from one node to another node in a cluster in almost
> real time.
>
> The problem is that CLOCK_MONOTONIC between nodes in the cluster
> has not relation ship to each other (except a synchronized length of
> the second). So applications that migrate can see CLOCK_MONOTONIC
> and CLOCK_BOOTTIME go backwards.
>
> This is the truly pressing problem and adding some kind of offset
> sounds like it would be the solution. Possibly by allowing a boot
> time synchronization of CLOCK_BOOTTIME and CLOCK_MONOTONIC.
>
> 2) Dealing with two separate time management domains. Say a machine
> that needes to deal with both something inside of google where they
> slew time to avoid leap time seconds and something in the outside
> world proper UTC time is kept as an offset from TAI with the
> occasional leap seconds.
>
> In the later case it would fundamentally require having seconds of
> different length.
>
I want to add that the second case should be optional.
When a container is migrated to another host, we have to restore its
monotonic and boottime clocks, but we still expect that the container
will continue using the host real-time clock.
Before stating this series, I was thinking about this, I decided that
these cases can be solved independently. Probably, the full isolation of
the time sub-system will have much higher overhead than just offsets for
a few clocks. And the idea that isolation of the real-time clock should
be optional gives us another hint that offsets for monotonic and
boot-time clocks can be implemented independently.
Eric and Tomas, what do you think about this? If you agree that these
two cases can be implemented separately, what should we do with this
series to make it ready to be merged?
I know that we need to:
* look at device drivers that report timestamps in CLOCK_MONOTONIC base.
* forbid changing offsets after creating timers
Anything else?
Thanks,
Andrei
>
> A pure 64bit nanoseond counter is good for 500 years. So 64bit
> variables can be used to hold time, and everything can be converted from
> there.
>
> This suggests we can for ticks have two values.
> - The number of ticks from the time source.
> - The number of times the ticks would have rolled over.
>
> That sounds like it may be a little simplistic as it would require being
> very diligent about firing a timer exactly at rollover and not losing
> that, but for a handwaving argument is probably enough to generate
> a 64bit tick counter.
>
> If the focus is on a 64bit tick counter then what update_wall_time
> has to do is very limited. Just deal the accounting needed to cope with
> tick rollover.
>
> Getting the actual time looks like it would be as simple as now, with
> perhaps an extra addition to account for the number of times the tick
> counter has rolled over. With limited precision arithmetic and various
> optimizations I don't think it is that simple to implement but it feels
> like it should be very little extra work.
>
> For timers my inclination would be to assume no adjustments to the
> current time parameters and set the timer to go off then. If the time
> on the appropriate clock has been changed since the timer was set and
> the timer is going off early reschedule so the timer fires at the
> appropriate time.
>
> With the above I think it is theoretically possible to build a time
> namespace that supports multiple lengths of second, and does not have
> much overhead.
>
> Not that I think a final implementation would necessary look like what I
> have described. I just think it is possible with extreme care to evolve
> the current code base into something that can efficiently handle
> multiple time domains with slightly different lenghts of second.
>
> Thomas does it sound like I am completely out of touch with reality?
>
> It does though sound like it is going to take some serious digging
> through the code to understand how what everything does and how and why
> everthing works the way it does. Not something grafted on top with just
> a cursory understanding of how the code works.
>
> Eric
> _______________________________________________
> Containers mailing list
> Containers@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers
^ permalink raw reply
* Re: [RFC 00/20] ns: Introduce Time Namespace
From: Andrei Vagin @ 2018-10-21 3:54 UTC (permalink / raw)
To: Eric W. Biederman, Thomas Gleixner
Cc: linux-kselftest@vger.kernel.org, Dmitry Safonov,
linux-api@vger.kernel.org, Jeff Dike, x86@kernel.org,
Dmitry Safonov, linux-kernel@vger.kernel.org, Oleg Nesterov,
criu@openvz.org, Ingo Molnar, Alexey Dobriyan, Andy Lutomirski,
H. Peter Anvin, Cyrill Gorcunov, Christian Brauner,
Pavel Emelianov, Shuah Khan,
containers@lists.linux-foundation.org
In-Reply-To: <20181021014121.GA23474@gmail.com>
On Sat, Oct 20, 2018 at 06:41:23PM -0700, Andrei Vagin wrote:
> On Fri, Sep 28, 2018 at 07:03:22PM +0200, Eric W. Biederman wrote:
> > Thomas Gleixner <tglx@linutronix.de> writes:
> >
> > > On Wed, 26 Sep 2018, Eric W. Biederman wrote:
> > >> Reading the code the calling sequence there is:
> > >> tick_sched_do_timer
> > >> tick_do_update_jiffies64
> > >> update_wall_time
> > >> timekeeping_advance
> > >> timekeepging_update
> > >>
> > >> If I read that properly under the right nohz circumstances that update
> > >> can be delayed indefinitely.
> > >>
> > >> So I think we could prototype a time namespace that was per
> > >> timekeeping_update and just had update_wall_time iterate through
> > >> all of the time namespaces.
> > >
> > > Please don't go there. timekeeping_update() is already heavy and walking
> > > through a gazillion of namespaces will just make it horrible,
> > >
> > >> I don't think the naive version would scale to very many time
> > >> namespaces.
> > >
> > > :)
> > >
> > >> At the same time using the techniques from the nohz work and a little
> > >> smarts I expect we could get the code to scale.
> > >
> > > You'd need to invoke the update when the namespace is switched in and
> > > hasn't been updated since the last tick happened. That might be doable, but
> > > you also need to take the wraparound constraints of the underlying
> > > clocksources into account, which again can cause walking all name spaces
> > > when they are all idle long enough.
> >
> > The wrap around constraints being how long before the time sources wrap
> > around so you have to read them once per wrap around? I have not dug
> > deeply enough into the code to see that yet.
> >
> > > From there it becomes hairy, because it's not only timekeeping,
> > > i.e. reading time, this is also affecting all timers which are armed from a
> > > namespace.
> > >
> > > That gets really ugly because when you do settimeofday() or adjtimex() for
> > > a particular namespace, then you have to search for all armed timers of
> > > that namespace and adjust them.
> > >
> > > The original posix timer code had the same issue because it mapped the
> > > clock realtime timers to the timer wheel so any setting of the clock caused
> > > a full walk of all armed timers, disarming, adjusting and requeing
> > > them. That's horrible not only performance wise, it's also a locking
> > > nightmare of all sorts.
> > >
> > > Add time skew via NTP/PTP into the picture and you might have to adjust
> > > timers as well, because you need to guarantee that they are not expiring
> > > early.
> > >
> > > I haven't looked through Dimitry's patches yet, but I don't see how this
> > > can work at all without introducing subtle issues all over the place.
> >
> > Then it sounds like this will take some more digging.
> >
> > Please pardon me for thinking out load.
> >
> > There are one or more time sources that we use to compute the time
> > and for each time source we have a conversion from ticks of the
> > time source to nanoseconds.
> >
> > Each time source needs to be sampled at least once per wrap-around
> > and something incremented so that we don't loose time when looking
> > at that time source.
> >
> > There are several clocks presented to userspace and they all share the
> > same length of second and are all fundamentally offsets from
> > CLOCK_MONOTONIC.
> >
> > I see two fundamental driving cases for a time namespace.
> > 1) Migration from one node to another node in a cluster in almost
> > real time.
> >
> > The problem is that CLOCK_MONOTONIC between nodes in the cluster
> > has not relation ship to each other (except a synchronized length of
> > the second). So applications that migrate can see CLOCK_MONOTONIC
> > and CLOCK_BOOTTIME go backwards.
> >
> > This is the truly pressing problem and adding some kind of offset
> > sounds like it would be the solution. Possibly by allowing a boot
> > time synchronization of CLOCK_BOOTTIME and CLOCK_MONOTONIC.
> >
> > 2) Dealing with two separate time management domains. Say a machine
> > that needes to deal with both something inside of google where they
> > slew time to avoid leap time seconds and something in the outside
> > world proper UTC time is kept as an offset from TAI with the
> > occasional leap seconds.
> >
> > In the later case it would fundamentally require having seconds of
> > different length.
> >
>
> I want to add that the second case should be optional.
>
> When a container is migrated to another host, we have to restore its
> monotonic and boottime clocks, but we still expect that the container
> will continue using the host real-time clock.
>
> Before stating this series, I was thinking about this, I decided that
> these cases can be solved independently. Probably, the full isolation of
> the time sub-system will have much higher overhead than just offsets for
> a few clocks. And the idea that isolation of the real-time clock should
> be optional gives us another hint that offsets for monotonic and
> boot-time clocks can be implemented independently.
>
> Eric and Tomas, what do you think about this? If you agree that these
Sorry Thomas, I mistyped your name.
> two cases can be implemented separately, what should we do with this
> series to make it ready to be merged?
>
> I know that we need to:
>
> * look at device drivers that report timestamps in CLOCK_MONOTONIC base.
> * forbid changing offsets after creating timers
>
> Anything else?
>
> Thanks,
> Andrei
>
> >
> > A pure 64bit nanoseond counter is good for 500 years. So 64bit
> > variables can be used to hold time, and everything can be converted from
> > there.
> >
> > This suggests we can for ticks have two values.
> > - The number of ticks from the time source.
> > - The number of times the ticks would have rolled over.
> >
> > That sounds like it may be a little simplistic as it would require being
> > very diligent about firing a timer exactly at rollover and not losing
> > that, but for a handwaving argument is probably enough to generate
> > a 64bit tick counter.
> >
> > If the focus is on a 64bit tick counter then what update_wall_time
> > has to do is very limited. Just deal the accounting needed to cope with
> > tick rollover.
> >
> > Getting the actual time looks like it would be as simple as now, with
> > perhaps an extra addition to account for the number of times the tick
> > counter has rolled over. With limited precision arithmetic and various
> > optimizations I don't think it is that simple to implement but it feels
> > like it should be very little extra work.
> >
> > For timers my inclination would be to assume no adjustments to the
> > current time parameters and set the timer to go off then. If the time
> > on the appropriate clock has been changed since the timer was set and
> > the timer is going off early reschedule so the timer fires at the
> > appropriate time.
> >
> > With the above I think it is theoretically possible to build a time
> > namespace that supports multiple lengths of second, and does not have
> > much overhead.
> >
> > Not that I think a final implementation would necessary look like what I
> > have described. I just think it is possible with extreme care to evolve
> > the current code base into something that can efficiently handle
> > multiple time domains with slightly different lenghts of second.
> >
> > Thomas does it sound like I am completely out of touch with reality?
> >
> > It does though sound like it is going to take some serious digging
> > through the code to understand how what everything does and how and why
> > everthing works the way it does. Not something grafted on top with just
> > a cursory understanding of how the code works.
> >
> > Eric
> > _______________________________________________
> > Containers mailing list
> > Containers@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/containers
^ permalink raw reply
* Re: [PATCH v7 1/6] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-10-21 16:04 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Linux Containers, Linux API, Andy Lutomirski, Oleg Nesterov,
Eric W . Biederman, Serge E . Hallyn, Christian Brauner,
Tyler Hicks, Akihiro Suda, Jann Horn,
linux-fsdevel@vger.kernel.org
In-Reply-To: <CAGXu5jK5yN7zcPUMzFYnUbRrVkLNE0YDCioYdNC3P6mF437+5g@mail.gmail.com>
On Wed, Oct 17, 2018 at 03:21:02PM -0700, Kees Cook wrote:
> On Wed, Oct 17, 2018 at 1:29 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> > On Thu, Sep 27, 2018 at 02:31:24PM -0700, Kees Cook wrote:
> >> On Thu, Sep 27, 2018 at 8:11 AM, Tycho Andersen <tycho@tycho.ws> wrote:
> >> > @@ -60,4 +62,29 @@ struct seccomp_data {
> >> > __u64 args[6];
> >> > };
> >> >
> >> > +struct seccomp_notif {
> >> > + __u16 len;
> >> > + __u64 id;
> >> > + __u32 pid;
> >> > + __u8 signaled;
> >> > + struct seccomp_data data;
> >> > +};
> >> > +
> >> > +struct seccomp_notif_resp {
> >> > + __u16 len;
> >> > + __u64 id;
> >> > + __s32 error;
> >> > + __s64 val;
> >> > +};
> >>
> >> So, len has to come first, for versioning. However, since it's ahead
> >> of a u64, this leaves a struct padding hole. pahole output:
> >>
> >> struct seccomp_notif {
> >> __u16 len; /* 0 2 */
> >>
> >> /* XXX 6 bytes hole, try to pack */
> >>
> >> __u64 id; /* 8 8 */
> >> __u32 pid; /* 16 4 */
> >> __u8 signaled; /* 20 1 */
> >>
> >> /* XXX 3 bytes hole, try to pack */
> >>
> >> struct seccomp_data data; /* 24 64 */
> >> /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
> >>
> >> /* size: 88, cachelines: 2, members: 5 */
> >> /* sum members: 79, holes: 2, sum holes: 9 */
> >> /* last cacheline: 24 bytes */
> >> };
> >> struct seccomp_notif_resp {
> >> __u16 len; /* 0 2 */
> >>
> >> /* XXX 6 bytes hole, try to pack */
> >>
> >> __u64 id; /* 8 8 */
> >> __s32 error; /* 16 4 */
> >>
> >> /* XXX 4 bytes hole, try to pack */
> >>
> >> __s64 val; /* 24 8 */
> >>
> >> /* size: 32, cachelines: 1, members: 4 */
> >> /* sum members: 22, holes: 2, sum holes: 10 */
> >> /* last cacheline: 32 bytes */
> >> };
> >>
> >> How about making len u32, and moving pid and error above "id"? This
> >> leaves a hole after signaled, so changing "len" won't be sufficient
> >> for versioning here. Perhaps move it after data?
> >
> > Just to confirm my understanding; I've got these as:
> >
> > struct seccomp_notif {
> > __u32 len; /* 0 4 */
> > __u32 pid; /* 4 4 */
> > __u64 id; /* 8 8 */
> > __u8 signaled; /* 16 1 */
> >
> > /* XXX 7 bytes hole, try to pack */
> >
> > struct seccomp_data data; /* 24 64 */
> > /* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
> >
> > /* size: 88, cachelines: 2, members: 5 */
> > /* sum members: 81, holes: 1, sum holes: 7 */
> > /* last cacheline: 24 bytes */
> > };
> > struct seccomp_notif_resp {
> > __u32 len; /* 0 4 */
> > __s32 error; /* 4 4 */
> > __u64 id; /* 8 8 */
> > __s64 val; /* 16 8 */
> >
> > /* size: 24, cachelines: 1, members: 4 */
> > /* last cacheline: 24 bytes */
> > };
> >
> > in the next version. Since the structure has no padding at the end of
> > it, I think the Right Thing will happen. Note that this is slightly
> > different than what Kees suggested, if I add signaled after data, then
> > I end up with:
> >
> > struct seccomp_notif {
> > __u32 len; /* 0 4 */
> > __u32 pid; /* 4 4 */
> > __u64 id; /* 8 8 */
> > struct seccomp_data data; /* 16 64 */
> > /* --- cacheline 1 boundary (64 bytes) was 16 bytes ago --- */
> > __u8 signaled; /* 80 1 */
> >
> > /* size: 88, cachelines: 2, members: 5 */
> > /* padding: 7 */
> > /* last cacheline: 24 bytes */
> > };
> >
> > which I think will have the versioning problem if the next member
> > introduces is < 7 bytes.
>
> It'll be a problem in either place. What I was thinking was that
> specific versioning is required instead of just length.
Euh, so I implemented this, and it sucks :). It's ugly, and generally
feels bad.
What if instead we just get rid of versioning all together, and
instead introduce a u32 flags? We could have one flag right now
(SECCOMP_NOTIF_FLAG_SIGNALED), and use introduce others as we add more
information to the response. Then we can add
SECCOMP_NOTIF_FLAG_EXTRA_FOO, and add another SECCOMP_IOCTL_GET_FOO to
grab the info?
FWIW, it's not really clear to me that we'll ever add anything to the
response since hopefully we'll land PUT_FD, so maybe this is all moot
anyway.
Tycho
^ permalink raw reply
* Re: [PATCH 01/34] vfs: syscall: Add open_tree(2) to reference or clone a mount [ver #12]
From: Eric W. Biederman @ 2018-10-21 16:41 UTC (permalink / raw)
To: David Howells
Cc: viro, linux-api, torvalds, linux-fsdevel, linux-kernel, mszeredi
In-Reply-To: <153754741974.17872.9547912893344172802.stgit@warthog.procyon.org.uk>
David Howells <dhowells@redhat.com> writes:
> From: Al Viro <viro@zeniv.linux.org.uk>
>
> open_tree(dfd, pathname, flags)
>
> Returns an O_PATH-opened file descriptor or an error.
> dfd and pathname specify the location to open, in usual
> fashion (see e.g. fstatat(2)). flags should be an OR of
> some of the following:
> * AT_PATH_EMPTY, AT_NO_AUTOMOUNT, AT_SYMLINK_NOFOLLOW -
> same meanings as usual
> * OPEN_TREE_CLOEXEC - make the resulting descriptor
> close-on-exec
> * OPEN_TREE_CLONE or OPEN_TREE_CLONE | AT_RECURSIVE -
> instead of opening the location in question, create a detached
> mount tree matching the subtree rooted at location specified by
> dfd/pathname. With AT_RECURSIVE the entire subtree is cloned,
> without it - only the part within in the mount containing the
> location in question. In other words, the same as mount --rbind
> or mount --bind would've taken. The detached tree will be
> dissolved on the final close of obtained file. Creation of such
> detached trees requires the same capabilities as doing mount --bind.
What happens when mounts propgate to such a detached mount tree?
It looks to me like the test in propagate_one for setting
CL_UNPRIVILEGED will trigger a NULL pointer dereference:
AKA:
/* Notice when we are propagating across user namespaces */
if (m->mnt_ns->user_ns != user_ns)
type |= CL_UNPRIVILEGED;
Since we don't know which mount namespace if any this subtree is going
into the test should become:
if (!m->mnt_ns || (m->mnt_ns->user_ns != user_ns))
type |= CL_UNPRIVILEGED;
If the tree is never attached anywhere it won't hurt as we don't allow
mounts or umounts on the detached subtree. We don't have enough
information to know about the namespace we copied from if it would cause
CL_UNPRIVILEGED to be set on any given propagation. Similarly we don't
have any information at all about the mount namespace this tree may
possibily be copied to.
Eric
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: linux-api@vger.kernel.org
> ---
>
> arch/x86/entry/syscalls/syscall_32.tbl | 1
> arch/x86/entry/syscalls/syscall_64.tbl | 1
> fs/file_table.c | 9 +-
> fs/internal.h | 1
> fs/namespace.c | 132 +++++++++++++++++++++++++++-----
> include/linux/fs.h | 7 +-
> include/linux/syscalls.h | 1
> include/uapi/linux/fcntl.h | 2
> include/uapi/linux/mount.h | 10 ++
> 9 files changed, 137 insertions(+), 27 deletions(-)
> create mode 100644 include/uapi/linux/mount.h
>
> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index 3cf7b533b3d1..ea1b413afd47 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -398,3 +398,4 @@
> 384 i386 arch_prctl sys_arch_prctl __ia32_compat_sys_arch_prctl
> 385 i386 io_pgetevents sys_io_pgetevents __ia32_compat_sys_io_pgetevents
> 386 i386 rseq sys_rseq __ia32_sys_rseq
> +387 i386 open_tree sys_open_tree __ia32_sys_open_tree
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index f0b1709a5ffb..0545bed581dc 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -343,6 +343,7 @@
> 332 common statx __x64_sys_statx
> 333 common io_pgetevents __x64_sys_io_pgetevents
> 334 common rseq __x64_sys_rseq
> +335 common open_tree __x64_sys_open_tree
>
> #
> # x32-specific system call numbers start at 512 to avoid cache impact
> diff --git a/fs/file_table.c b/fs/file_table.c
> index e49af4caf15d..e03c8d121c6c 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -255,6 +255,7 @@ static void __fput(struct file *file)
> struct dentry *dentry = file->f_path.dentry;
> struct vfsmount *mnt = file->f_path.mnt;
> struct inode *inode = file->f_inode;
> + fmode_t mode = file->f_mode;
>
> if (unlikely(!(file->f_mode & FMODE_OPENED)))
> goto out;
> @@ -277,18 +278,20 @@ static void __fput(struct file *file)
> if (file->f_op->release)
> file->f_op->release(inode, file);
> if (unlikely(S_ISCHR(inode->i_mode) && inode->i_cdev != NULL &&
> - !(file->f_mode & FMODE_PATH))) {
> + !(mode & FMODE_PATH))) {
> cdev_put(inode->i_cdev);
> }
> fops_put(file->f_op);
> put_pid(file->f_owner.pid);
> - if ((file->f_mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
> + if ((mode & (FMODE_READ | FMODE_WRITE)) == FMODE_READ)
> i_readcount_dec(inode);
> - if (file->f_mode & FMODE_WRITER) {
> + if (mode & FMODE_WRITER) {
> put_write_access(inode);
> __mnt_drop_write(mnt);
> }
> dput(dentry);
> + if (unlikely(mode & FMODE_NEED_UNMOUNT))
> + dissolve_on_fput(mnt);
> mntput(mnt);
> out:
> file_free(file);
> diff --git a/fs/internal.h b/fs/internal.h
> index 364c20b5ea2d..17029b30e196 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -85,6 +85,7 @@ extern int __mnt_want_write_file(struct file *);
> extern void __mnt_drop_write(struct vfsmount *);
> extern void __mnt_drop_write_file(struct file *);
>
> +extern void dissolve_on_fput(struct vfsmount *);
> /*
> * fs_struct.c
> */
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 8a7e1a7d1d06..ded1a970ec40 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -20,12 +20,14 @@
> #include <linux/init.h> /* init_rootfs */
> #include <linux/fs_struct.h> /* get_fs_root et.al. */
> #include <linux/fsnotify.h> /* fsnotify_vfsmount_delete */
> +#include <linux/file.h>
> #include <linux/uaccess.h>
> #include <linux/proc_ns.h>
> #include <linux/magic.h>
> #include <linux/bootmem.h>
> #include <linux/task_work.h>
> #include <linux/sched/task.h>
> +#include <uapi/linux/mount.h>
>
> #include "pnode.h"
> #include "internal.h"
> @@ -1779,6 +1781,16 @@ struct vfsmount *collect_mounts(const struct path *path)
> return &tree->mnt;
> }
>
> +void dissolve_on_fput(struct vfsmount *mnt)
> +{
> + namespace_lock();
> + lock_mount_hash();
> + mntget(mnt);
> + umount_tree(real_mount(mnt), UMOUNT_CONNECTED);
> + unlock_mount_hash();
> + namespace_unlock();
> +}
> +
> void drop_collected_mounts(struct vfsmount *mnt)
> {
> namespace_lock();
> @@ -2138,6 +2150,30 @@ static bool has_locked_children(struct mount *mnt, struct dentry *dentry)
> return false;
> }
>
> +static struct mount *__do_loopback(struct path *old_path, int recurse)
> +{
> + struct mount *mnt = ERR_PTR(-EINVAL), *old = real_mount(old_path->mnt);
> +
> + if (IS_MNT_UNBINDABLE(old))
> + return mnt;
> +
> + if (!check_mnt(old) && old_path->dentry->d_op != &ns_dentry_operations)
> + return mnt;
> +
> + if (!recurse && has_locked_children(old, old_path->dentry))
> + return mnt;
> +
> + if (recurse)
> + mnt = copy_tree(old, old_path->dentry, CL_COPY_MNT_NS_FILE);
> + else
> + mnt = clone_mnt(old, old_path->dentry, 0);
> +
> + if (!IS_ERR(mnt))
> + mnt->mnt.mnt_flags &= ~MNT_LOCKED;
> +
> + return mnt;
> +}
> +
> /*
> * do loopback mount.
> */
> @@ -2145,7 +2181,7 @@ static int do_loopback(struct path *path, const char *old_name,
> int recurse)
> {
> struct path old_path;
> - struct mount *mnt = NULL, *old, *parent;
> + struct mount *mnt = NULL, *parent;
> struct mountpoint *mp;
> int err;
> if (!old_name || !*old_name)
> @@ -2159,38 +2195,21 @@ static int do_loopback(struct path *path, const char *old_name,
> goto out;
>
> mp = lock_mount(path);
> - err = PTR_ERR(mp);
> - if (IS_ERR(mp))
> + if (IS_ERR(mp)) {
> + err = PTR_ERR(mp);
> goto out;
> + }
>
> - old = real_mount(old_path.mnt);
> parent = real_mount(path->mnt);
> -
> - err = -EINVAL;
> - if (IS_MNT_UNBINDABLE(old))
> - goto out2;
> -
> if (!check_mnt(parent))
> goto out2;
>
> - if (!check_mnt(old) && old_path.dentry->d_op != &ns_dentry_operations)
> - goto out2;
> -
> - if (!recurse && has_locked_children(old, old_path.dentry))
> - goto out2;
> -
> - if (recurse)
> - mnt = copy_tree(old, old_path.dentry, CL_COPY_MNT_NS_FILE);
> - else
> - mnt = clone_mnt(old, old_path.dentry, 0);
> -
> + mnt = __do_loopback(&old_path, recurse);
> if (IS_ERR(mnt)) {
> err = PTR_ERR(mnt);
> goto out2;
> }
>
> - mnt->mnt.mnt_flags &= ~MNT_LOCKED;
> -
> err = graft_tree(mnt, parent, mp);
> if (err) {
> lock_mount_hash();
> @@ -2204,6 +2223,75 @@ static int do_loopback(struct path *path, const char *old_name,
> return err;
> }
>
> +SYSCALL_DEFINE3(open_tree, int, dfd, const char *, filename, unsigned, flags)
> +{
> + struct file *file;
> + struct path path;
> + int lookup_flags = LOOKUP_AUTOMOUNT | LOOKUP_FOLLOW;
> + bool detached = flags & OPEN_TREE_CLONE;
> + int error;
> + int fd;
> +
> + BUILD_BUG_ON(OPEN_TREE_CLOEXEC != O_CLOEXEC);
> +
> + if (flags & ~(AT_EMPTY_PATH | AT_NO_AUTOMOUNT | AT_RECURSIVE |
> + AT_SYMLINK_NOFOLLOW | OPEN_TREE_CLONE |
> + OPEN_TREE_CLOEXEC))
> + return -EINVAL;
> +
> + if ((flags & (AT_RECURSIVE | OPEN_TREE_CLONE)) == AT_RECURSIVE)
> + return -EINVAL;
> +
> + if (flags & AT_NO_AUTOMOUNT)
> + lookup_flags &= ~LOOKUP_AUTOMOUNT;
> + if (flags & AT_SYMLINK_NOFOLLOW)
> + lookup_flags &= ~LOOKUP_FOLLOW;
> + if (flags & AT_EMPTY_PATH)
> + lookup_flags |= LOOKUP_EMPTY;
> +
> + if (detached && !may_mount())
> + return -EPERM;
> +
> + fd = get_unused_fd_flags(flags & O_CLOEXEC);
> + if (fd < 0)
> + return fd;
> +
> + error = user_path_at(dfd, filename, lookup_flags, &path);
> + if (error)
> + goto out;
> +
> + if (detached) {
> + struct mount *mnt = __do_loopback(&path, flags & AT_RECURSIVE);
> + if (IS_ERR(mnt)) {
> + error = PTR_ERR(mnt);
> + goto out2;
> + }
> + mntput(path.mnt);
> + path.mnt = &mnt->mnt;
> + }
> +
> + file = dentry_open(&path, O_PATH, current_cred());
> + if (IS_ERR(file)) {
> + error = PTR_ERR(file);
> + goto out3;
> + }
> +
> + if (detached)
> + file->f_mode |= FMODE_NEED_UNMOUNT;
> + path_put(&path);
> + fd_install(fd, file);
> + return fd;
> +
> +out3:
> + if (detached)
> + dissolve_on_fput(path.mnt);
> +out2:
> + path_put(&path);
> +out:
> + put_unused_fd(fd);
> + return error;
> +}
> +
> static int change_mount_flags(struct vfsmount *mnt, int ms_flags)
> {
> int error = 0;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 4323b8fe353d..6dc32507762f 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -157,10 +157,13 @@ typedef int (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
> #define FMODE_NONOTIFY ((__force fmode_t)0x4000000)
>
> /* File is capable of returning -EAGAIN if I/O will block */
> -#define FMODE_NOWAIT ((__force fmode_t)0x8000000)
> +#define FMODE_NOWAIT ((__force fmode_t)0x8000000)
> +
> +/* File represents mount that needs unmounting */
> +#define FMODE_NEED_UNMOUNT ((__force fmode_t)0x10000000)
>
> /* File does not contribute to nr_files count */
> -#define FMODE_NOACCOUNT ((__force fmode_t)0x20000000)
> +#define FMODE_NOACCOUNT ((__force fmode_t)0x20000000)
>
> /*
> * Flag for rw_copy_check_uvector and compat_rw_copy_check_uvector
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 2ff814c92f7f..6978f3c76d41 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -906,6 +906,7 @@ asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
> unsigned mask, struct statx __user *buffer);
> asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
> int flags, uint32_t sig);
> +asmlinkage long sys_open_tree(int dfd, const char __user *path, unsigned flags);
>
> /*
> * Architecture-specific system calls
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index 6448cdd9a350..594b85f7cb86 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -90,5 +90,7 @@
> #define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
> #define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
>
> +#define AT_RECURSIVE 0x8000 /* Apply to the entire subtree */
> +
>
> #endif /* _UAPI_LINUX_FCNTL_H */
> diff --git a/include/uapi/linux/mount.h b/include/uapi/linux/mount.h
> new file mode 100644
> index 000000000000..e8db2911adca
> --- /dev/null
> +++ b/include/uapi/linux/mount.h
> @@ -0,0 +1,10 @@
> +#ifndef _UAPI_LINUX_MOUNT_H
> +#define _UAPI_LINUX_MOUNT_H
> +
> +/*
> + * open_tree() flags.
> + */
> +#define OPEN_TREE_CLONE 1 /* Clone the target tree and attach the clone */
> +#define OPEN_TREE_CLOEXEC O_CLOEXEC /* Close the file on execve() */
> +
> +#endif /* _UAPI_LINUX_MOUNT_H */
^ permalink raw reply
* Re: [PATCH 1/3] printk: Introduce per-console loglevel setting
From: Sergey Senozhatsky @ 2018-10-22 2:37 UTC (permalink / raw)
To: Calvin Owens
Cc: Sergey Senozhatsky, Petr Mladek, Sergey Senozhatsky,
Steven Rostedt, linux-api@vger.kernel.org,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
Kernel Team
In-Reply-To: <20181019220314.GA8164@Haydn>
On (10/19/18 22:03), Calvin Owens wrote:
> Hi Sergey,
>
> It's in-progress, I'm sorry it hasn't happened sooner.
Great!
> By embedding the kobject in the console struct, we end up needing to refactor
> the console drivers to use the kobject refcount instead of simply calling
> kfree(), which is what I'm working on. It ends up being tedious but not
> particularly complicated, my goal is to have this up soon :)
OK :)
-ss
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox