* Re: [RFC PATCH ghak32 V2 04/13] audit: add containerid filtering
From: Paul Moore @ 2018-04-19 0:24 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <b933f93762435990e9b1e6d5aebf15f186ac8951.1521179281.git.rgb@redhat.com>
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Implement container ID filtering using the AUDIT_CONTAINERID field name
> to send an 8-character string representing a u64 since the value field
> is only u32.
>
> Sending it as two u32 was considered, but gathering and comparing two
> fields was more complex.
My only worry here is that you aren't really sending a string in the
ASCII sense, you are sending an 8 byte buffer (that better be NUL
terminated) that happens to be an unsigned 64-bit integer. To be
clear, I'm okay with that (it's protected by AUDIT_CONTAINERID), and
the code is okay with that, I just want us to pause for a minute and
make sure that is an okay thing to do long term.
> The feature indicator is AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER.
>
> This requires support from userspace to be useful.
> See: https://github.com/linux-audit/audit-userspace/issues/40
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 1 +
> include/uapi/linux/audit.h | 5 ++++-
> kernel/audit.h | 1 +
> kernel/auditfilter.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
> kernel/auditsc.c | 3 +++
> 5 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 3acbe9d..f10ca1b 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -76,6 +76,7 @@ struct audit_field {
> u32 type;
> union {
> u32 val;
> + u64 val64;
> kuid_t uid;
> kgid_t gid;
> struct {
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index e83ccbd..8443a8f 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -262,6 +262,7 @@
> #define AUDIT_LOGINUID_SET 24
> #define AUDIT_SESSIONID 25 /* Session ID */
> #define AUDIT_FSTYPE 26 /* FileSystem Type */
> +#define AUDIT_CONTAINERID 27 /* Container ID */
>
> /* These are ONLY useful when checking
> * at syscall exit time (AUDIT_AT_EXIT). */
> @@ -342,6 +343,7 @@ enum {
> #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> #define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
> +#define AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER 0x00000080
>
> #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
> @@ -349,7 +351,8 @@ enum {
> AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
> AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
> AUDIT_FEATURE_BITMAP_LOST_RESET | \
> - AUDIT_FEATURE_BITMAP_FILTER_FS)
> + AUDIT_FEATURE_BITMAP_FILTER_FS | \
> + AUDIT_FEATURE_BITMAP_CONTAINERID_FILTER)
>
> /* deprecated: AUDIT_VERSION_* */
> #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 214e149..aaa651a 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -234,6 +234,7 @@ static inline int audit_hash_ino(u32 ino)
>
> extern int audit_match_class(int class, unsigned syscall);
> extern int audit_comparator(const u32 left, const u32 op, const u32 right);
> +extern int audit_comparator64(const u64 left, const u32 op, const u64 right);
> extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right);
> extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right);
> extern int parent_len(const char *path);
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index d7a807e..c4c8746 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -410,6 +410,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
> /* FALL THROUGH */
> case AUDIT_ARCH:
> case AUDIT_FSTYPE:
> + case AUDIT_CONTAINERID:
> if (f->op != Audit_not_equal && f->op != Audit_equal)
> return -EINVAL;
> break;
> @@ -584,6 +585,14 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
> }
> entry->rule.exe = audit_mark;
> break;
> + case AUDIT_CONTAINERID:
> + if (f->val != sizeof(u64))
> + goto exit_free;
> + str = audit_unpack_string(&bufp, &remain, f->val);
> + if (IS_ERR(str))
> + goto exit_free;
> + f->val64 = ((u64 *)str)[0];
> + break;
> }
> }
>
> @@ -666,6 +675,11 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
> data->buflen += data->values[i] =
> audit_pack_string(&bufp, audit_mark_path(krule->exe));
> break;
> + case AUDIT_CONTAINERID:
> + data->buflen += data->values[i] = sizeof(u64);
> + for (i = 0; i < sizeof(u64); i++)
> + ((char *)bufp)[i] = ((char *)&f->val64)[i];
> + break;
> case AUDIT_LOGINUID_SET:
> if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {
> data->fields[i] = AUDIT_LOGINUID;
> @@ -752,6 +766,10 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
> if (!gid_eq(a->fields[i].gid, b->fields[i].gid))
> return 1;
> break;
> + case AUDIT_CONTAINERID:
> + if (a->fields[i].val64 != b->fields[i].val64)
> + return 1;
> + break;
> default:
> if (a->fields[i].val != b->fields[i].val)
> return 1;
> @@ -1210,6 +1228,31 @@ int audit_comparator(u32 left, u32 op, u32 right)
> }
> }
>
> +int audit_comparator64(u64 left, u32 op, u64 right)
> +{
> + switch (op) {
> + case Audit_equal:
> + return (left == right);
> + case Audit_not_equal:
> + return (left != right);
> + case Audit_lt:
> + return (left < right);
> + case Audit_le:
> + return (left <= right);
> + case Audit_gt:
> + return (left > right);
> + case Audit_ge:
> + return (left >= right);
> + case Audit_bitmask:
> + return (left & right);
> + case Audit_bittest:
> + return ((left & right) == right);
> + default:
> + BUG();
> + return 0;
> + }
> +}
> +
> int audit_uid_comparator(kuid_t left, u32 op, kuid_t right)
> {
> switch (op) {
> @@ -1348,6 +1391,10 @@ int audit_filter(int msgtype, unsigned int listtype)
> result = audit_comparator(audit_loginuid_set(current),
> f->op, f->val);
> break;
> + case AUDIT_CONTAINERID:
> + result = audit_comparator64(audit_get_containerid(current),
> + f->op, f->val64);
> + break;
> case AUDIT_MSGTYPE:
> result = audit_comparator(msgtype, f->op, f->val);
> break;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 65be110..2bba324 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -614,6 +614,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> case AUDIT_LOGINUID_SET:
> result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);
> break;
> + case AUDIT_CONTAINERID:
> + result = audit_comparator64(audit_get_containerid(tsk), f->op, f->val64);
> + break;
> case AUDIT_SUBJ_USER:
> case AUDIT_SUBJ_ROLE:
> case AUDIT_SUBJ_TYPE:
> --
> 1.8.3.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 02/13] audit: check children and threading before allowing containerid
From: Paul Moore @ 2018-04-19 0:11 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: simo-H+wXaHxf7aLQT0dZR+AlfA, jlayton-H+wXaHxf7aLQT0dZR+AlfA,
carlos-H+wXaHxf7aLQT0dZR+AlfA, linux-api-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, LKML,
Eric Paris, dhowells-H+wXaHxf7aLQT0dZR+AlfA,
Linux-Audit Mailing List, ebiederm-aS9lmoZGLiVWk0Htik3J/w,
luto-DgEjT+Ai2ygdnm+yROfE0A, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
cgroups-u79uwXL29TY76Z2rM5mHXA,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn
In-Reply-To: <995b77557010b2f9aed0e10435f7b8536df7a5db.1521179281.git.rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Check if a task has existing children or co-threads and refuse to set
> the container ID if either are present. Failure to check this could
> permit games where a child scratches its parent's back to work around
> inheritance and double-setting policy.
>
> Signed-off-by: Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> kernel/auditsc.c | 4 ++++
> 1 file changed, 4 insertions(+)
I would just include this in patch 1/2 as I can't think of world where
we wouldn't this check.
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 29c8482..a6b0a52 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2087,6 +2087,10 @@ static int audit_set_containerid_perm(struct task_struct *task, u64 containerid)
> /* if we don't have caps, reject */
> if (!capable(CAP_AUDIT_CONTROL))
> return -EPERM;
> + /* if task has children or is not single-threaded, deny */
> + if (!list_empty(&task->children) ||
> + !(thread_group_leader(task) && thread_group_empty(task)))
> + return -EPERM;
> /* if containerid is unset, allow */
> if (!audit_containerid_set(task))
> return 0;
> --
> 1.8.3.1
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
From: Paul Moore @ 2018-04-18 23:47 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, ebiederm, luto, jlayton, carlos,
dhowells, viro, simo, Eric Paris, serge
In-Reply-To: <e284617ad667ad8f17958dd8babb87fe1b4d7205.1521179281.git.rgb@redhat.com>
On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> Implement the proc fs write to set the audit container ID of a process,
> emitting an AUDIT_CONTAINER record to document the event.
>
> This is a write from the container orchestrator task to a proc entry of
> the form /proc/PID/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).
>
> This will produce a record such as this:
> type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
>
> 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 container ID values are given in the
> "contid" fields, while res indicates its success.
>
> It is not permitted to self-set, unset or re-set the container ID. A
> child inherits its parent's container ID, but then can be set only once
> after.
>
> See: https://github.com/linux-audit/audit-kernel/issues/32
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> fs/proc/base.c | 37 ++++++++++++++++++++
> include/linux/audit.h | 16 +++++++++
> include/linux/init_task.h | 4 ++-
> include/linux/sched.h | 1 +
> include/uapi/linux/audit.h | 2 ++
> kernel/auditsc.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 143 insertions(+), 1 deletion(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 60316b5..6ce4fbe 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file * file, char __user * buf,
> .read = proc_sessionid_read,
> .llseek = generic_file_llseek,
> };
> +
> +static ssize_t proc_containerid_write(struct file *file, const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + struct inode *inode = file_inode(file);
> + u64 containerid;
> + int rv;
> + struct task_struct *task = get_proc_task(inode);
> +
> + if (!task)
> + return -ESRCH;
> + if (*ppos != 0) {
> + /* No partial writes. */
> + put_task_struct(task);
> + return -EINVAL;
> + }
> +
> + rv = kstrtou64_from_user(buf, count, 10, &containerid);
> + if (rv < 0) {
> + put_task_struct(task);
> + return rv;
> + }
> +
> + rv = audit_set_containerid(task, containerid);
> + put_task_struct(task);
> + if (rv < 0)
> + return rv;
> + return count;
> +}
> +
> +static const struct file_operations proc_containerid_operations = {
> + .write = proc_containerid_write,
> + .llseek = generic_file_llseek,
> +};
> +
> #endif
>
> #ifdef CONFIG_FAULT_INJECTION
> @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file *m, struct pid_namespace *ns,
> #ifdef CONFIG_AUDITSYSCALL
> REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> REG("sessionid", S_IRUGO, proc_sessionid_operations),
> + REG("containerid", S_IWUSR, proc_containerid_operations),
> #endif
> #ifdef CONFIG_FAULT_INJECTION
> REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> @@ -3355,6 +3391,7 @@ static int proc_tid_comm_permission(struct inode *inode, int mask)
> #ifdef CONFIG_AUDITSYSCALL
> REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> REG("sessionid", S_IRUGO, proc_sessionid_operations),
> + REG("containerid", S_IWUSR, proc_containerid_operations),
> #endif
> #ifdef CONFIG_FAULT_INJECTION
> REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index af410d9..fe4ba3f 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -29,6 +29,7 @@
>
> #define AUDIT_INO_UNSET ((unsigned long)-1)
> #define AUDIT_DEV_UNSET ((dev_t)-1)
> +#define INVALID_CID AUDIT_CID_UNSET
Why can't we just use AUDIT_CID_UNSET? Is there an important
distinction? If so, they shouldn't they have different values?
If we do need to keep INVALID_CID, let's rename it to
AUDIT_CID_INVALID so we have some consistency to the naming patterns
and we stress that it is an *audit* container ID.
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index d258826..1b82191 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -796,6 +796,7 @@ struct task_struct {
> #ifdef CONFIG_AUDITSYSCALL
> kuid_t loginuid;
> unsigned int sessionid;
> + u64 containerid;
This one line addition to the task_struct scares me the most of
anything in this patchset. Why? It's a field named "containerid" in
a perhaps one of the most widely used core kernel structures; the
possibilities for abuse are endless, and it's foolish to think we
would ever be able to adequately police this.
Unfortunately, we can't add the field to audit_context as things
currently stand because we don't always allocate an audit_context,
it's dependent on the system's configuration, and we need to track the
audit container ID for a given process, regardless of the audit
configuration. Pretty much the same reason why loginuid and sessionid
are located directly in task_struct now. As I stressed during the
design phase, I really want to keep this as an *audit* container ID
and not a general purpose kernel wide container ID. If the kernel
ever grows a general purpose container ID token, I'll be the first in
line to convert the audit code, but I don't want audit to be that
general purpose mechanism ... audit is hated enough as-is ;)
I think the right solution to this is to create another new struct,
audit_task_info (or similar, the name really isn't that important),
which would be stored as a pointer in task_struct and would replace
the audit_context pointer, loginuid, sessionid, and the newly proposed
containerid. The new audit_task_info would always be allocated in the
audit_alloc() function (please use kmem_cache), and the audit_context
pointer included inside would continue to be allocated based on the
existing conditions. By keeping audit_task_info as a pointer inside
task_struct we could hide the structure definition inside
kernel/audit*.c and make it much more difficult for other subsystems
to abuse it.[1]
struct audit_task_info {
kuid_t loginuid;
unsigned int sessionid;
u64 containerid;
struct audit_context *ctx;
}
Actually, we might even want to consider storing audit_context in
audit_task_info (no pointer), or making it a zero length array
(ctx[0]) and going with a variable sized allocation of audit_task_info
... but all that could be done as a follow up optimization once we get
the basic idea sorted.
[1] If for some reason allocating audit_task_info becomes too much
overhead to bear (somewhat doubtful since we would only do it at task
creation), we could do some ugly tricks to directly include an
audit_task_struct chunk in task_struct but I'd like to avoid that if
possible (and I think we can).
> #endif
> struct seccomp seccomp;
...
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 4e61a9e..921a71f 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -71,6 +71,7 @@
> #define AUDIT_TTY_SET 1017 /* Set TTY auditing status */
> #define AUDIT_SET_FEATURE 1018 /* Turn an audit feature on or off */
> #define AUDIT_GET_FEATURE 1019 /* Get which features are enabled */
> +#define AUDIT_CONTAINER 1020 /* Define the container id and information */
>
> #define AUDIT_FIRST_USER_MSG 1100 /* Userspace messages mostly uninteresting to kernel */
> #define AUDIT_USER_AVC 1107 /* We filter this differently */
> @@ -465,6 +466,7 @@ struct audit_tty_status {
> };
>
> #define AUDIT_UID_UNSET (unsigned int)-1
> +#define AUDIT_CID_UNSET ((u64)-1)
I think we need to decide if we want to distinguish between the "host"
(e.g. init ns) and "unset". Looking at this patch (I've only quickly
skimmed the others so far) it would appear that you don't think we
need to worry about this distinction; that's fine, but let's make it
explicit with a comment in the code that AUDIT_CID_UNSET means "unset"
as well as "host".
If we do need to make a distinction, let's add a constant/macro for "host".
> /* audit_rule_data supports filter rules with both integer and string
> * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 4e0a4ac..29c8482 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2073,6 +2073,90 @@ int audit_set_loginuid(kuid_t loginuid)
> return rc;
> }
>
> +static int audit_set_containerid_perm(struct task_struct *task, u64 containerid)
> +{
> + struct task_struct *parent;
> + u64 pcontainerid, ccontainerid;
> +
> + /* Don't allow to set our own containerid */
> + if (current == task)
> + return -EPERM;
Why not? Is there some obvious security concern that I missing?
I ask because I suppose it might be possible for some container
runtime to do a fork, setup some of the environment and them exec the
container (before you answer the obvious "namespaces!" please remember
we're not trying to define containers).
> + /* Don't allow the containerid to be unset */
> + if (!cid_valid(containerid))
> + return -EINVAL;
> + /* if we don't have caps, reject */
> + if (!capable(CAP_AUDIT_CONTROL))
> + return -EPERM;
> + /* if containerid is unset, allow */
> + if (!audit_containerid_set(task))
> + return 0;
> + /* it is already set, and not inherited from the parent, reject */
> + ccontainerid = audit_get_containerid(task);
> + rcu_read_lock();
> + parent = rcu_dereference(task->real_parent);
> + rcu_read_unlock();
> + task_lock(parent);
> + pcontainerid = audit_get_containerid(parent);
> + task_unlock(parent);
> + if (ccontainerid != pcontainerid)
> + return -EPERM;
> + return 0;
> +}
> +
> +static void audit_log_set_containerid(struct task_struct *task, u64 oldcontainerid,
> + u64 containerid, int rc)
> +{
> + struct audit_buffer *ab;
> + uid_t uid;
> + struct tty_struct *tty;
> +
> + if (!audit_enabled)
> + return;
> +
> + ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONTAINER);
> + if (!ab)
> + return;
> +
> + uid = from_kuid(&init_user_ns, task_uid(current));
> + tty = audit_get_tty(current);
> +
> + audit_log_format(ab, "op=set pid=%d uid=%u", task_tgid_nr(current), uid);
> + audit_log_task_context(ab);
> + audit_log_format(ab, " auid=%u tty=%s ses=%u opid=%d old-contid=%llu contid=%llu res=%d",
> + from_kuid(&init_user_ns, audit_get_loginuid(current)),
> + tty ? tty_name(tty) : "(none)", audit_get_sessionid(current),
> + task_tgid_nr(task), oldcontainerid, containerid, !rc);
> +
> + audit_put_tty(tty);
> + audit_log_end(ab);
> +}
> +
> +/**
> + * audit_set_containerid - set current task's audit_context containerid
> + * @containerid: containerid value
> + *
> + * Returns 0 on success, -EPERM on permission failure.
> + *
> + * Called (set) from fs/proc/base.c::proc_containerid_write().
> + */
> +int audit_set_containerid(struct task_struct *task, u64 containerid)
> +{
> + u64 oldcontainerid;
> + int rc;
> +
> + oldcontainerid = audit_get_containerid(task);
> +
> + rc = audit_set_containerid_perm(task, containerid);
> + if (!rc) {
> + task_lock(task);
> + task->containerid = containerid;
> + task_unlock(task);
> + }
> +
> + audit_log_set_containerid(task, oldcontainerid, containerid, rc);
> + return rc;
Why are audit_set_containerid_perm() and audit_log_containerid()
separate functions?
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [RFC PATCH V1 01/12] audit: add container id
From: Richard Guy Briggs @ 2018-04-18 19:51 UTC (permalink / raw)
To: Stefan Berger
Cc: mszeredi, ebiederm, simo, jlayton, carlos, linux-api, containers,
LKML, eparis, dhowells, Linux-Audit Mailing List, viro, luto,
netdev, linux-fsdevel, cgroups, serge, trondmy
In-Reply-To: <c1ec93a2-b398-373c-55da-b2be8e60c6b6@linux.vnet.ibm.com>
On 2018-04-18 15:39, Stefan Berger wrote:
> On 04/18/2018 03:23 PM, Richard Guy Briggs wrote:
> > On 2018-04-18 14:45, Stefan Berger wrote:
> > > On 03/15/2018 11:58 PM, Richard Guy Briggs wrote:
> > > > On 2018-03-15 16:27, Stefan Berger wrote:
> > > > > On 03/01/2018 02:41 PM, Richard Guy Briggs wrote:
> > > > > > Implement the proc fs write to set the audit container ID of a process,
> > > > > > emitting an AUDIT_CONTAINER record to document the event.
> > > > > >
> > > > > > This is a write from the container orchestrator task to a proc entry of
> > > > > > the form /proc/PID/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).
> > > > > >
> > > > > > This will produce a record such as this:
> > > > > > type=UNKNOWN[1333] msg=audit(1519903238.968:261): op=set pid=596 uid=0 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
> > > > > >
> > > > > > 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 container ID values are given in the
> > > > > > "contid" fields, while res indicates its success.
> > > > > >
> > > > > > It is not permitted to self-set, unset or re-set the container ID. A
> > > > > > child inherits its parent's container ID, but then can be set only once
> > > > > > after.
> > > > > >
> > > > > > See: https://github.com/linux-audit/audit-kernel/issues/32
> > > > > >
> > > > > >
> > > > > > /* audit_rule_data supports filter rules with both integer and string
> > > > > > * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
> > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > > index 4e0a4ac..0ee1e59 100644
> > > > > > --- a/kernel/auditsc.c
> > > > > > +++ b/kernel/auditsc.c
> > > > > > @@ -2073,6 +2073,92 @@ int audit_set_loginuid(kuid_t loginuid)
> > > > > > return rc;
> > > > > > }
> > > > > >
> > > > > > +static int audit_set_containerid_perm(struct task_struct *task, u64 containerid)
> > > > > > +{
> > > > > > + struct task_struct *parent;
> > > > > > + u64 pcontainerid, ccontainerid;
> > > > > > + pid_t ppid;
> > > > > > +
> > > > > > + /* Don't allow to set our own containerid */
> > > > > > + if (current == task)
> > > > > > + return -EPERM;
> > > > > > + /* Don't allow the containerid to be unset */
> > > > > > + if (!cid_valid(containerid))
> > > > > > + return -EINVAL;
> > > > > > + /* if we don't have caps, reject */
> > > > > > + if (!capable(CAP_AUDIT_CONTROL))
> > > > > > + return -EPERM;
> > > > > > + /* if containerid is unset, allow */
> > > > > > + if (!audit_containerid_set(task))
> > > > > > + return 0;
> > > > > I am wondering whether there should be a check for the target process that
> > > > > will receive the containerid to not have CAP_SYS_ADMIN that would otherwise
> > > > > allow it to arbitrarily unshare()/clone() and leave the set of namespaces
> > > > > that may make up the container whose containerid we assign here?
> > > > This is a reasonable question. This has been debated and I understood
> > > > the conclusion was that without a clear definition of a "container", the
> > > > task still remains in that container that just now has more
> > > > sub-namespaces (in the case of hierarchical namespaces), we don't want
> > > > to restrict it in such a way and that allows it to create nested
> > > > containers. I see setns being more problematic if it could switch to
> > > > another existing namespace that was set up by the orchestrator for a
> > > > different container. The coming v2 patchset acknowledges this situation
> > > > with the network namespace being potentially shared by multiple
> > > > containers.
> > > Are you going to post v2 soon? We would like to build on top of it for IMA
> > > namespacing and auditing inside of IMA namespaces.
> > I don't know if it addresses your specific needs, but V2 was posted on
> > March 16th along with userspace patches:
> > https://www.redhat.com/archives/linux-audit/2018-March/msg00110.html
> > https://www.redhat.com/archives/linux-audit/2018-March/msg00124.html
> >
> > V3 is pending.
> Thanks. I hadn't actually looked at primarily due to the ghak and ghau in
> the title. Whatever these may mean.
They are Github issue numbers:
GHAK: GitHub Audit Kernel
GHAU: GitHub Audit Userspace
GHAD: GitHub Audit Documentation
GHAT: GitHub Audit Testsuite
> Does V2 or will V3 prevent a privileged process to setns() to a whole
> different set of namespaces and still be audited with that initial container
> id ?
No, not significantly different from V1 in that respect.
It does not prevent setns(), but will maintain its containerid.
It will prevent games by blocking a child and parent from setting each
other's containerids.
It does check that the task being conainered does not yet have any
children or peer threads.
- 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: [RFC PATCH V1 01/12] audit: add container id
From: Stefan Berger @ 2018-04-18 19:39 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: mszeredi, ebiederm, simo, jlayton, carlos, linux-api, containers,
LKML, eparis, dhowells, Linux-Audit Mailing List, viro, luto,
netdev, linux-fsdevel, cgroups, serge, trondmy
In-Reply-To: <20180418192359.n4q53bvsdhrjftjg@madcap2.tricolour.ca>
On 04/18/2018 03:23 PM, Richard Guy Briggs wrote:
> On 2018-04-18 14:45, Stefan Berger wrote:
>> On 03/15/2018 11:58 PM, Richard Guy Briggs wrote:
>>> On 2018-03-15 16:27, Stefan Berger wrote:
>>>> On 03/01/2018 02:41 PM, Richard Guy Briggs wrote:
>>>>> Implement the proc fs write to set the audit container ID of a process,
>>>>> emitting an AUDIT_CONTAINER record to document the event.
>>>>>
>>>>> This is a write from the container orchestrator task to a proc entry of
>>>>> the form /proc/PID/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).
>>>>>
>>>>> This will produce a record such as this:
>>>>> type=UNKNOWN[1333] msg=audit(1519903238.968:261): op=set pid=596 uid=0 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
>>>>>
>>>>> 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 container ID values are given in the
>>>>> "contid" fields, while res indicates its success.
>>>>>
>>>>> It is not permitted to self-set, unset or re-set the container ID. A
>>>>> child inherits its parent's container ID, but then can be set only once
>>>>> after.
>>>>>
>>>>> See: https://github.com/linux-audit/audit-kernel/issues/32
>>>>>
>>>>>
>>>>> /* audit_rule_data supports filter rules with both integer and string
>>>>> * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
>>>>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>>>>> index 4e0a4ac..0ee1e59 100644
>>>>> --- a/kernel/auditsc.c
>>>>> +++ b/kernel/auditsc.c
>>>>> @@ -2073,6 +2073,92 @@ int audit_set_loginuid(kuid_t loginuid)
>>>>> return rc;
>>>>> }
>>>>>
>>>>> +static int audit_set_containerid_perm(struct task_struct *task, u64 containerid)
>>>>> +{
>>>>> + struct task_struct *parent;
>>>>> + u64 pcontainerid, ccontainerid;
>>>>> + pid_t ppid;
>>>>> +
>>>>> + /* Don't allow to set our own containerid */
>>>>> + if (current == task)
>>>>> + return -EPERM;
>>>>> + /* Don't allow the containerid to be unset */
>>>>> + if (!cid_valid(containerid))
>>>>> + return -EINVAL;
>>>>> + /* if we don't have caps, reject */
>>>>> + if (!capable(CAP_AUDIT_CONTROL))
>>>>> + return -EPERM;
>>>>> + /* if containerid is unset, allow */
>>>>> + if (!audit_containerid_set(task))
>>>>> + return 0;
>>>> I am wondering whether there should be a check for the target process that
>>>> will receive the containerid to not have CAP_SYS_ADMIN that would otherwise
>>>> allow it to arbitrarily unshare()/clone() and leave the set of namespaces
>>>> that may make up the container whose containerid we assign here?
>>> This is a reasonable question. This has been debated and I understood
>>> the conclusion was that without a clear definition of a "container", the
>>> task still remains in that container that just now has more
>>> sub-namespaces (in the case of hierarchical namespaces), we don't want
>>> to restrict it in such a way and that allows it to create nested
>>> containers. I see setns being more problematic if it could switch to
>>> another existing namespace that was set up by the orchestrator for a
>>> different container. The coming v2 patchset acknowledges this situation
>>> with the network namespace being potentially shared by multiple
>>> containers.
>> Are you going to post v2 soon? We would like to build on top of it for IMA
>> namespacing and auditing inside of IMA namespaces.
> I don't know if it addresses your specific needs, but V2 was posted on
> March 16th along with userspace patches:
> https://www.redhat.com/archives/linux-audit/2018-March/msg00110.html
> https://www.redhat.com/archives/linux-audit/2018-March/msg00124.html
>
> V3 is pending.
Thanks. I hadn't actually looked at primarily due to the ghak and ghau
in the title. Whatever these may mean.
Does V2 or will V3 prevent a privileged process to setns() to a whole
different set of namespaces and still be audited with that initial
container id ?
^ permalink raw reply
* Re: [RFC PATCH V1 01/12] audit: add container id
From: Richard Guy Briggs @ 2018-04-18 19:23 UTC (permalink / raw)
To: Stefan Berger
Cc: mszeredi, ebiederm, simo, jlayton, carlos, linux-api, containers,
LKML, eparis, dhowells, Linux-Audit Mailing List, viro, luto,
netdev, linux-fsdevel, cgroups, serge, trondmy
In-Reply-To: <f966fa52-da4b-3d74-0848-1f0b08e57fd9@linux.vnet.ibm.com>
On 2018-04-18 14:45, Stefan Berger wrote:
> On 03/15/2018 11:58 PM, Richard Guy Briggs wrote:
> > On 2018-03-15 16:27, Stefan Berger wrote:
> > > On 03/01/2018 02:41 PM, Richard Guy Briggs wrote:
> > > > Implement the proc fs write to set the audit container ID of a process,
> > > > emitting an AUDIT_CONTAINER record to document the event.
> > > >
> > > > This is a write from the container orchestrator task to a proc entry of
> > > > the form /proc/PID/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).
> > > >
> > > > This will produce a record such as this:
> > > > type=UNKNOWN[1333] msg=audit(1519903238.968:261): op=set pid=596 uid=0 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
> > > >
> > > > 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 container ID values are given in the
> > > > "contid" fields, while res indicates its success.
> > > >
> > > > It is not permitted to self-set, unset or re-set the container ID. A
> > > > child inherits its parent's container ID, but then can be set only once
> > > > after.
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/32
> > > >
> > > >
> > > > /* audit_rule_data supports filter rules with both integer and string
> > > > * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index 4e0a4ac..0ee1e59 100644
> > > > --- a/kernel/auditsc.c
> > > > +++ b/kernel/auditsc.c
> > > > @@ -2073,6 +2073,92 @@ int audit_set_loginuid(kuid_t loginuid)
> > > > return rc;
> > > > }
> > > >
> > > > +static int audit_set_containerid_perm(struct task_struct *task, u64 containerid)
> > > > +{
> > > > + struct task_struct *parent;
> > > > + u64 pcontainerid, ccontainerid;
> > > > + pid_t ppid;
> > > > +
> > > > + /* Don't allow to set our own containerid */
> > > > + if (current == task)
> > > > + return -EPERM;
> > > > + /* Don't allow the containerid to be unset */
> > > > + if (!cid_valid(containerid))
> > > > + return -EINVAL;
> > > > + /* if we don't have caps, reject */
> > > > + if (!capable(CAP_AUDIT_CONTROL))
> > > > + return -EPERM;
> > > > + /* if containerid is unset, allow */
> > > > + if (!audit_containerid_set(task))
> > > > + return 0;
> > > I am wondering whether there should be a check for the target process that
> > > will receive the containerid to not have CAP_SYS_ADMIN that would otherwise
> > > allow it to arbitrarily unshare()/clone() and leave the set of namespaces
> > > that may make up the container whose containerid we assign here?
> > This is a reasonable question. This has been debated and I understood
> > the conclusion was that without a clear definition of a "container", the
> > task still remains in that container that just now has more
> > sub-namespaces (in the case of hierarchical namespaces), we don't want
> > to restrict it in such a way and that allows it to create nested
> > containers. I see setns being more problematic if it could switch to
> > another existing namespace that was set up by the orchestrator for a
> > different container. The coming v2 patchset acknowledges this situation
> > with the network namespace being potentially shared by multiple
> > containers.
>
> Are you going to post v2 soon? We would like to build on top of it for IMA
> namespacing and auditing inside of IMA namespaces.
I don't know if it addresses your specific needs, but V2 was posted on
March 16th along with userspace patches:
https://www.redhat.com/archives/linux-audit/2018-March/msg00110.html
https://www.redhat.com/archives/linux-audit/2018-March/msg00124.html
V3 is pending.
> Stefan
- 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
* sendmmsg flags userspace ABI change in kernel 4.6
From: Florian Weimer @ 2018-04-18 19:03 UTC (permalink / raw)
To: linux-api, netdev, linux-sctp; +Cc: linux-kernel, Tom Herbert, mjw
Since this commit:
commit 28a94d8fb35b3a75b802f368ae6f4a9f6b0d435a
Author: Tom Herbert <tom@herbertland.com>
Date: Mon Mar 7 14:11:02 2016 -0800
net: Allow MSG_EOR in each msghdr of sendmmsg
This patch allows setting MSG_EOR in each individual msghdr passed
in sendmmsg. This allows a sendmmsg to send multiple messages when
using SOCK_SEQPACKET.
Signed-off-by: Tom Herbert <tom@herbertland.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
the msg_flags argument in individual msghdr arguments is longer
completely ignored for SOCK_SEQPACKET sockets. msg_flags was and is
still documented as ignored for sendmsg(2), so by analogy for
sendmmsg(2) as well.
It seems that valgrind does not know about this yet, and due to
limited use of SCTP, this userspace ABI change has not been noticed so
far.
What are the plans in this area? Will other kinds of sockets start
using the msghdr flags for sending?
A fully backwards-compatibility way to achieve this would be to
specify that you have to pass a new flag to sendmmsg (MSG_PERHDR?), in
its flags argument, to activate the per-msghdr flags.
The glibc DNS stub resolver relies on the previously documented
behavior, and I wonder how widely we should backport the change:
https://sourceware.org/bugzilla/show_bug.cgi?id=23037
If the MSG_PERHDR route will be taken, we can skip this work, and
valgrind can flag uninitialized bits in msg_flags only if MSG_PERHDR
is passed. (I believe it would be difficult for valgrind to look at
the socket type to determine whether undefined bits need reporting.)
^ permalink raw reply
* Re: [RFC PATCH V1 01/12] audit: add container id
From: Stefan Berger @ 2018-04-18 18:45 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups, containers, linux-api, Linux-Audit Mailing List,
linux-fsdevel, LKML, netdev, mszeredi, luto, jlayton, carlos,
viro, dhowells, simo, trondmy, eparis, serge, ebiederm, madzcar
In-Reply-To: <20180316035837.ddnqvbyrbp3fdk7e@madcap2.tricolour.ca>
On 03/15/2018 11:58 PM, Richard Guy Briggs wrote:
> On 2018-03-15 16:27, Stefan Berger wrote:
>> On 03/01/2018 02:41 PM, Richard Guy Briggs wrote:
>>> Implement the proc fs write to set the audit container ID of a process,
>>> emitting an AUDIT_CONTAINER record to document the event.
>>>
>>> This is a write from the container orchestrator task to a proc entry of
>>> the form /proc/PID/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).
>>>
>>> This will produce a record such as this:
>>> type=UNKNOWN[1333] msg=audit(1519903238.968:261): op=set pid=596 uid=0 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0 ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
>>>
>>> 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 container ID values are given in the
>>> "contid" fields, while res indicates its success.
>>>
>>> It is not permitted to self-set, unset or re-set the container ID. A
>>> child inherits its parent's container ID, but then can be set only once
>>> after.
>>>
>>> See: https://github.com/linux-audit/audit-kernel/issues/32
>>>
>>>
>>> /* audit_rule_data supports filter rules with both integer and string
>>> * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and
>>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>>> index 4e0a4ac..0ee1e59 100644
>>> --- a/kernel/auditsc.c
>>> +++ b/kernel/auditsc.c
>>> @@ -2073,6 +2073,92 @@ int audit_set_loginuid(kuid_t loginuid)
>>> return rc;
>>> }
>>>
>>> +static int audit_set_containerid_perm(struct task_struct *task, u64 containerid)
>>> +{
>>> + struct task_struct *parent;
>>> + u64 pcontainerid, ccontainerid;
>>> + pid_t ppid;
>>> +
>>> + /* Don't allow to set our own containerid */
>>> + if (current == task)
>>> + return -EPERM;
>>> + /* Don't allow the containerid to be unset */
>>> + if (!cid_valid(containerid))
>>> + return -EINVAL;
>>> + /* if we don't have caps, reject */
>>> + if (!capable(CAP_AUDIT_CONTROL))
>>> + return -EPERM;
>>> + /* if containerid is unset, allow */
>>> + if (!audit_containerid_set(task))
>>> + return 0;
>> I am wondering whether there should be a check for the target process that
>> will receive the containerid to not have CAP_SYS_ADMIN that would otherwise
>> allow it to arbitrarily unshare()/clone() and leave the set of namespaces
>> that may make up the container whose containerid we assign here?
> This is a reasonable question. This has been debated and I understood
> the conclusion was that without a clear definition of a "container", the
> task still remains in that container that just now has more
> sub-namespaces (in the case of hierarchical namespaces), we don't want
> to restrict it in such a way and that allows it to create nested
> containers. I see setns being more problematic if it could switch to
> another existing namespace that was set up by the orchestrator for a
> different container. The coming v2 patchset acknowledges this situation
> with the network namespace being potentially shared by multiple
> containers.
Are you going to post v2 soon? We would like to build on top of it for
IMA namespacing and auditing inside of IMA namespaces.
Stefan
^ permalink raw reply
* Re: [PATCH 2/3] mm: add find_alloc_contig_pages() interface
From: Mike Kravetz @ 2018-04-18 1:39 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, linux-mm, linux-kernel, linux-api, Reinette Chatre,
Michal Hocko, Christopher Lameter, Guy Shattah, Anshuman Khandual,
Michal Nazarewicz, Vlastimil Babka, David Nellans, Laura Abbott,
Pavel Machek, Dave Hansen, Andrew Morton
In-Reply-To: <201804172011.K5f3XeGz%fengguang.wu@intel.com>
On 04/17/2018 05:10 AM, kbuild test robot wrote:
> All errors (new ones prefixed by >>):
>
> In file included from include/linux/slab.h:15:0,
> from include/linux/crypto.h:24,
> from arch/x86/kernel/asm-offsets.c:9:
>>> include/linux/gfp.h:580:15: error: unknown type name 'page'
> static inline page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
> ^~~~
> include/linux/gfp.h:585:13: warning: 'free_contig_pages' defined but not used [-Wunused-function]
> static void free_contig_pages(struct page *page, unsigned long nr_pages)
> ^~~~~~~~~~~~~~~~~
Build issues fixed in updated patch below,
>From 28efabb5625c079573a821c8c5cfc19cc73a86bd Mon Sep 17 00:00:00 2001
From: Mike Kravetz <mike.kravetz@oracle.com>
Date: Mon, 16 Apr 2018 18:41:36 -0700
Subject: [PATCH 2/3] mm: add find_alloc_contig_pages() interface
find_alloc_contig_pages() is a new interface that attempts to locate
and allocate a contiguous range of pages. It is provided as a more
convenient interface than alloc_contig_range() which is currently
used by CMA and gigantic huge pages.
When attempting to allocate a range of pages, migration is employed
if possible. There is no guarantee that the routine will succeed.
So, the user must be prepared for failure and have a fall back plan.
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
include/linux/gfp.h | 12 ++++++++
mm/page_alloc.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 99 insertions(+), 2 deletions(-)
diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 86a0d06463ab..7d1ea4e659dc 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -573,6 +573,18 @@ static inline bool pm_suspended_storage(void)
extern int alloc_contig_range(unsigned long start, unsigned long end,
unsigned migratetype, gfp_t gfp_mask);
extern void free_contig_range(unsigned long pfn, unsigned long nr_pages);
+extern struct page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
+ int nid, nodemask_t *nodemask);
+extern void free_contig_pages(struct page *page, unsigned long nr_pages);
+#else
+static inline struct page *find_alloc_contig_pages(unsigned int order,
+ gfp_t gfp, int nid, nodemask_t *nodemask)
+{
+ return NULL;
+}
+static inline void free_contig_pages(struct page *page, unsigned long nr_pages)
+{
+}
#endif
#ifdef CONFIG_CMA
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0fd5e8e2456e..81070fe55c44 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -67,6 +67,7 @@
#include <linux/ftrace.h>
#include <linux/lockdep.h>
#include <linux/nmi.h>
+#include <linux/mmzone.h>
#include <asm/sections.h>
#include <asm/tlbflush.h>
@@ -2010,9 +2011,13 @@ static __always_inline struct page *__rmqueue_cma_fallback(struct zone *zone,
{
return __rmqueue_smallest(zone, order, MIGRATE_CMA);
}
+#define contig_alloc_migratetype_ok(migratetype) \
+ ((migratetype) == MIGRATE_CMA || (migratetype) == MIGRATE_MOVABLE)
#else
static inline struct page *__rmqueue_cma_fallback(struct zone *zone,
unsigned int order) { return NULL; }
+#define contig_alloc_migratetype_ok(migratetype) \
+ ((migratetype) == MIGRATE_MOVABLE)
#endif
/*
@@ -7822,6 +7827,9 @@ int alloc_contig_range(unsigned long start, unsigned long end,
};
INIT_LIST_HEAD(&cc.migratepages);
+ if (!contig_alloc_migratetype_ok(migratetype))
+ return -EINVAL;
+
/*
* What we do here is we mark all pageblocks in range as
* MIGRATE_ISOLATE. Because pageblock and max order pages may
@@ -7912,8 +7920,9 @@ int alloc_contig_range(unsigned long start, unsigned long end,
/* Make sure the range is really isolated. */
if (test_pages_isolated(outer_start, end, false)) {
- pr_info_ratelimited("%s: [%lx, %lx) PFNs busy\n",
- __func__, outer_start, end);
+ if (!(migratetype == MIGRATE_MOVABLE)) /* only print for CMA */
+ pr_info_ratelimited("%s: [%lx, %lx) PFNs busy\n",
+ __func__, outer_start, end);
ret = -EBUSY;
goto done;
}
@@ -7949,6 +7958,82 @@ void free_contig_range(unsigned long pfn, unsigned long nr_pages)
}
WARN(count != 0, "%ld pages are still in use!\n", count);
}
+
+static bool contig_pfn_range_valid(struct zone *z, unsigned long start_pfn,
+ unsigned long nr_pages)
+{
+ unsigned long i, end_pfn = start_pfn + nr_pages;
+ struct page *page;
+
+ for (i = start_pfn; i < end_pfn; i++) {
+ if (!pfn_valid(i))
+ return false;
+
+ page = pfn_to_page(i);
+
+ if (page_zone(page) != z)
+ return false;
+
+ }
+
+ return true;
+}
+
+/**
+ * find_alloc_contig_pages() -- attempt to find and allocate a contiguous
+ * range of pages
+ * @order: number of pages
+ * @gfp: gfp mask used to limit search as well as during compaction
+ * @nid: target node
+ * @nodemask: mask of other possible nodes
+ *
+ * Pages can be freed with a call to free_contig_pages(), or by manually
+ * calling __free_page() for each page allocated.
+ *
+ * Return: pointer to 'order' pages on success, or NULL if not successful.
+ */
+struct page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
+ int nid, nodemask_t *nodemask)
+{
+ unsigned long pfn, nr_pages, flags;
+ struct page *ret_page = NULL;
+ struct zonelist *zonelist;
+ struct zoneref *z;
+ struct zone *zone;
+ int rc;
+
+ nr_pages = 1 << order;
+ zonelist = node_zonelist(nid, gfp);
+ for_each_zone_zonelist_nodemask(zone, z, zonelist, gfp_zone(gfp),
+ nodemask) {
+ spin_lock_irqsave(&zone->lock, flags);
+ pfn = ALIGN(zone->zone_start_pfn, nr_pages);
+ while (zone_spans_pfn(zone, pfn + nr_pages - 1)) {
+ if (contig_pfn_range_valid(zone, pfn, nr_pages)) {
+ spin_unlock_irqrestore(&zone->lock, flags);
+
+ rc = alloc_contig_range(pfn, pfn + nr_pages,
+ MIGRATE_MOVABLE, gfp);
+ if (!rc) {
+ ret_page = pfn_to_page(pfn);
+ return ret_page;
+ }
+ spin_lock_irqsave(&zone->lock, flags);
+ }
+ pfn += nr_pages;
+ }
+ spin_unlock_irqrestore(&zone->lock, flags);
+ }
+
+ return ret_page;
+}
+EXPORT_SYMBOL_GPL(find_alloc_contig_pages);
+
+void free_contig_pages(struct page *page, unsigned long nr_pages)
+{
+ free_contig_range(page_to_pfn(page), nr_pages);
+}
+EXPORT_SYMBOL_GPL(free_contig_pages);
#endif
#if defined CONFIG_MEMORY_HOTPLUG || defined CONFIG_CMA
--
2.13.6
^ permalink raw reply related
* Re: [RFC PATCH] fs: introduce ST_HUGE flag and set it to tmpfs and hugetlbfs
From: Yang Shi @ 2018-04-17 21:51 UTC (permalink / raw)
To: Andrew Morton
Cc: viro, nyc, mike.kravetz, kirill.shutemov, hughd, linux-fsdevel,
linux-mm, linux-kernel, linux-man, mtk.manpages, linux-api
In-Reply-To: <20180417143144.b7ffb07fad28875bad546247@linux-foundation.org>
On 4/17/18 2:31 PM, Andrew Morton wrote:
> On Wed, 18 Apr 2018 05:08:13 +0800 Yang Shi <yang.shi@linux.alibaba.com> wrote:
>
>> Since tmpfs THP was supported in 4.8, hugetlbfs is not the only
>> filesystem with huge page support anymore. tmpfs can use huge page via
>> THP when mounting by "huge=" mount option.
>>
>> When applications use huge page on hugetlbfs, it just need check the
>> filesystem magic number, but it is not enough for tmpfs. So, introduce
>> ST_HUGE flag to statfs if super block has SB_HUGE set which indicates
>> huge page is supported on the specific filesystem.
>>
>> Some applications could benefit from this change, for example QEMU.
>> When use mmap file as guest VM backend memory, QEMU typically mmap the
>> file size plus one extra page. If the file is on hugetlbfs the extra
>> page is huge page size (i.e. 2MB), but it is still 4KB on tmpfs even
>> though THP is enabled. tmpfs THP requires VMA is huge page aligned, so
>> if 4KB page is used THP will not be used at all. The below /proc/meminfo
>> fragment shows the THP use of QEMU with 4K page:
>>
>> ShmemHugePages: 679936 kB
>> ShmemPmdMapped: 0 kB
>>
>> With ST_HUGE flag, QEMU can get huge page, then /proc/meminfo looks
>> like:
>>
>> ShmemHugePages: 77824 kB
>> ShmemPmdMapped: 6144 kB
>>
>> With this flag, the applications can know if huge page is supported on
>> the filesystem then optimize the behavior of the applications
>> accordingly. Although the similar function can be implemented in
>> applications by traversing the mount options, it looks more convenient
>> if kernel can provide such flag.
>>
>> Even though ST_HUGE is set, f_bsize still returns 4KB for tmpfs since
>> THP could be split, and it also my fallback to 4KB page silently if
>> there is not enough huge page.
>>
>> And, set the flag for hugetlbfs as well to keep the consistency, and the
>> applications don't have to know what filesystem is used to use huge
>> page, just need to check ST_HUGE flag.
>>
> Patch is simple enough, although I'm having trouble forming an opinion
> about it ;)
>
> It will call for an update to the statfs(2) manpage. I'm not sure
> which of linux-man@vger.kernel.org, mtk.manpages@gmail.com and
> linux-api@vger.kernel.org is best for that, so I'd cc all three...
Thanks, Andrew. Added cc to those 3 lists.
^ permalink raw reply
* Re: [PATCH 2/3] mm: add find_alloc_contig_pages() interface
From: kbuild test robot @ 2018-04-17 14:19 UTC (permalink / raw)
Cc: kbuild-all, linux-mm, linux-kernel, linux-api, Reinette Chatre,
Michal Hocko, Christopher Lameter, Guy Shattah, Anshuman Khandual,
Michal Nazarewicz, Vlastimil Babka, David Nellans, Laura Abbott,
Pavel Machek, Dave Hansen, Andrew Morton, Mike Kravetz
In-Reply-To: <20180417020915.11786-3-mike.kravetz@oracle.com>
[-- Attachment #1: Type: text/plain, Size: 2853 bytes --]
Hi Mike,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on mmotm/master]
[also build test ERROR on v4.17-rc1 next-20180417]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Mike-Kravetz/mm-change-type-of-free_contig_range-nr_pages-to-unsigned-long/20180417-194309
base: git://git.cmpxchg.org/linux-mmotm.git master
config: sparc-defconfig (attached as .config)
compiler: sparc-linux-gcc (GCC) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc
All errors (new ones prefixed by >>):
In file included from include/linux/slab.h:15:0,
from include/linux/irq.h:21,
from include/asm-generic/hardirq.h:13,
from arch/sparc/include/asm/hardirq_32.h:11,
from arch/sparc/include/asm/hardirq.h:7,
from include/linux/hardirq.h:9,
from include/linux/interrupt.h:11,
from include/linux/kernel_stat.h:9,
from arch/sparc/kernel/irq_32.c:15:
include/linux/gfp.h:580:15: error: unknown type name 'page'
static inline page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
^~~~
>> include/linux/gfp.h:585:13: error: 'free_contig_pages' defined but not used [-Werror=unused-function]
static void free_contig_pages(struct page *page, unsigned long nr_pages)
^~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
vim +/free_contig_pages +585 include/linux/gfp.h
570
571 #if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || defined(CONFIG_CMA)
572 /* The below functions must be run on a range from a single zone. */
573 extern int alloc_contig_range(unsigned long start, unsigned long end,
574 unsigned migratetype, gfp_t gfp_mask);
575 extern void free_contig_range(unsigned long pfn, unsigned long nr_pages);
576 extern struct page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
577 int nid, nodemask_t *nodemask);
578 extern void free_contig_pages(struct page *page, unsigned long nr_pages);
579 #else
> 580 static inline page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
581 int nid, nodemask_t *nodemask)
582 {
583 return NULL;
584 }
> 585 static void free_contig_pages(struct page *page, unsigned long nr_pages)
586 {
587 }
588 #endif
589
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 11547 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] mm: add find_alloc_contig_pages() interface
From: kbuild test robot @ 2018-04-17 12:10 UTC (permalink / raw)
Cc: kbuild-all, linux-mm, linux-kernel, linux-api, Reinette Chatre,
Michal Hocko, Christopher Lameter, Guy Shattah, Anshuman Khandual,
Michal Nazarewicz, Vlastimil Babka, David Nellans, Laura Abbott,
Pavel Machek, Dave Hansen, Andrew Morton, Mike Kravetz
In-Reply-To: <20180417020915.11786-3-mike.kravetz@oracle.com>
[-- Attachment #1: Type: text/plain, Size: 2543 bytes --]
Hi Mike,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on mmotm/master]
[also build test ERROR on v4.17-rc1 next-20180417]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Mike-Kravetz/mm-change-type-of-free_contig_range-nr_pages-to-unsigned-long/20180417-194309
base: git://git.cmpxchg.org/linux-mmotm.git master
config: i386-tinyconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
In file included from include/linux/slab.h:15:0,
from include/linux/crypto.h:24,
from arch/x86/kernel/asm-offsets.c:9:
>> include/linux/gfp.h:580:15: error: unknown type name 'page'
static inline page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
^~~~
include/linux/gfp.h:585:13: warning: 'free_contig_pages' defined but not used [-Wunused-function]
static void free_contig_pages(struct page *page, unsigned long nr_pages)
^~~~~~~~~~~~~~~~~
make[2]: *** [arch/x86/kernel/asm-offsets.s] Error 1
make[2]: Target '__build' not remade because of errors.
make[1]: *** [prepare0] Error 2
make[1]: Target 'prepare' not remade because of errors.
make: *** [sub-make] Error 2
vim +/page +580 include/linux/gfp.h
570
571 #if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || defined(CONFIG_CMA)
572 /* The below functions must be run on a range from a single zone. */
573 extern int alloc_contig_range(unsigned long start, unsigned long end,
574 unsigned migratetype, gfp_t gfp_mask);
575 extern void free_contig_range(unsigned long pfn, unsigned long nr_pages);
576 extern struct page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
577 int nid, nodemask_t *nodemask);
578 extern void free_contig_pages(struct page *page, unsigned long nr_pages);
579 #else
> 580 static inline page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
581 int nid, nodemask_t *nodemask)
582 {
583 return NULL;
584 }
585 static void free_contig_pages(struct page *page, unsigned long nr_pages)
586 {
587 }
588 #endif
589
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6311 bytes --]
^ permalink raw reply
* Re: [PATCH] mmap.2: MAP_FIXED is okay if the address range has been reserved
From: Michal Hocko @ 2018-04-17 6:23 UTC (permalink / raw)
To: Jann Horn
Cc: Michael Kerrisk (man-pages), John Hubbard, linux-man,
Andrew Morton, Linux-MM, lkml, Linux API
In-Reply-To: <CAG48ez2fXcQS7sw_aCC6_wBKjYphOxFRN4rrRzSk2+-T_mFaxw@mail.gmail.com>
On Mon 16-04-18 23:12:48, Jann Horn wrote:
> On Mon, Apr 16, 2018 at 11:11 PM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Mon 16-04-18 22:17:40, Jann Horn wrote:
> >> On Mon, Apr 16, 2018 at 9:57 PM, Michal Hocko <mhocko@kernel.org> wrote:
> >> > On Mon 16-04-18 21:30:09, Jann Horn wrote:
> >> >> On Mon, Apr 16, 2018 at 9:18 PM, Michal Hocko <mhocko@kernel.org> wrote:
> >> > [...]
> >> >> > Yes, reasonably well written application will not have this problem.
> >> >> > That, however, requires an external synchronization and that's why
> >> >> > called it error prone and racy. I guess that was the main motivation for
> >> >> > that part of the man page.
> >> >>
> >> >> What requires external synchronization? I still don't understand at
> >> >> all what you're talking about.
> >> >>
> >> >> The following code:
> >> >>
> >> >> void *try_to_alloc_addr(void *hint, size_t len) {
> >> >> char *x = mmap(hint, len, ...);
> >> >> if (x == MAP_FAILED) return NULL;
> >> >> if (x == hint) return x;
> >> >
> >> > Any other thread can modify the address space at this moment.
> >>
> >> But not parts of the address space that were returned by this mmap() call.
> > ?
> >> > Just
> >> > consider that another thread would does mmap(x, MAP_FIXED) (or any other
> >> > address overlapping [x, x+len] range)
> >>
> >> If the other thread does that without previously having created a
> >> mapping covering the area in question, that would be a bug in the
> >> other thread.
> >
> > MAP_FIXED is sometimes used without preallocated address ranges.
>
> Wow, really? Can you point to an example?
Just from top of my head.
Some of that is for historical reasons because the hint address used to
be ignored on some operating systems so MAP_FIXED had to be used.
Currently not user I guess but MAP_FIXED for addresses above 47b address
space AFAIR.
And I am pretty sure there would be much more if you actually browsed
code search.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* [PATCH 3/3] mm/hugetlb: use find_alloc_contig_pages() to allocate gigantic pages
From: Mike Kravetz @ 2018-04-17 2:09 UTC (permalink / raw)
To: linux-mm, linux-kernel, linux-api
Cc: Reinette Chatre, Michal Hocko, Christopher Lameter, Guy Shattah,
Anshuman Khandual, Michal Nazarewicz, Vlastimil Babka,
David Nellans, Laura Abbott, Pavel Machek, Dave Hansen,
Andrew Morton, Mike Kravetz
In-Reply-To: <20180417020915.11786-1-mike.kravetz@oracle.com>
Use the new find_alloc_contig_pages() interface for the allocation of
gigantic pages and remove associated code in hugetlb.c.
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
mm/hugetlb.c | 87 +++++-------------------------------------------------------
1 file changed, 6 insertions(+), 81 deletions(-)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index c81072ce7510..a209767cb808 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1053,91 +1053,16 @@ static void destroy_compound_gigantic_page(struct page *page,
__ClearPageHead(page);
}
-static void free_gigantic_page(struct page *page, unsigned int order)
+static void free_gigantic_page(struct page *page, struct hstate *h)
{
- free_contig_range(page_to_pfn(page), 1UL << order);
-}
-
-static int __alloc_gigantic_page(unsigned long start_pfn,
- unsigned long nr_pages, gfp_t gfp_mask)
-{
- unsigned long end_pfn = start_pfn + nr_pages;
- return alloc_contig_range(start_pfn, end_pfn, MIGRATE_MOVABLE,
- gfp_mask);
-}
-
-static bool pfn_range_valid_gigantic(struct zone *z,
- unsigned long start_pfn, unsigned long nr_pages)
-{
- unsigned long i, end_pfn = start_pfn + nr_pages;
- struct page *page;
-
- for (i = start_pfn; i < end_pfn; i++) {
- if (!pfn_valid(i))
- return false;
-
- page = pfn_to_page(i);
-
- if (page_zone(page) != z)
- return false;
-
- if (PageReserved(page))
- return false;
-
- if (page_count(page) > 0)
- return false;
-
- if (PageHuge(page))
- return false;
- }
-
- return true;
-}
-
-static bool zone_spans_last_pfn(const struct zone *zone,
- unsigned long start_pfn, unsigned long nr_pages)
-{
- unsigned long last_pfn = start_pfn + nr_pages - 1;
- return zone_spans_pfn(zone, last_pfn);
+ free_contig_pages(page, (unsigned long)pages_per_huge_page(h));
}
static struct page *alloc_gigantic_page(struct hstate *h, gfp_t gfp_mask,
int nid, nodemask_t *nodemask)
{
- unsigned int order = huge_page_order(h);
- unsigned long nr_pages = 1 << order;
- unsigned long ret, pfn, flags;
- struct zonelist *zonelist;
- struct zone *zone;
- struct zoneref *z;
-
- zonelist = node_zonelist(nid, gfp_mask);
- for_each_zone_zonelist_nodemask(zone, z, zonelist, gfp_zone(gfp_mask), nodemask) {
- spin_lock_irqsave(&zone->lock, flags);
-
- pfn = ALIGN(zone->zone_start_pfn, nr_pages);
- while (zone_spans_last_pfn(zone, pfn, nr_pages)) {
- if (pfn_range_valid_gigantic(zone, pfn, nr_pages)) {
- /*
- * We release the zone lock here because
- * alloc_contig_range() will also lock the zone
- * at some point. If there's an allocation
- * spinning on this lock, it may win the race
- * and cause alloc_contig_range() to fail...
- */
- spin_unlock_irqrestore(&zone->lock, flags);
- ret = __alloc_gigantic_page(pfn, nr_pages, gfp_mask);
- if (!ret)
- return pfn_to_page(pfn);
- spin_lock_irqsave(&zone->lock, flags);
- }
- pfn += nr_pages;
- }
-
- spin_unlock_irqrestore(&zone->lock, flags);
- }
-
- return NULL;
+ return find_alloc_contig_pages(huge_page_order(h), gfp_mask, nid,
+ nodemask);
}
static void prep_new_huge_page(struct hstate *h, struct page *page, int nid);
@@ -1147,7 +1072,7 @@ static void prep_compound_gigantic_page(struct page *page, unsigned int order);
static inline bool gigantic_page_supported(void) { return false; }
static struct page *alloc_gigantic_page(struct hstate *h, gfp_t gfp_mask,
int nid, nodemask_t *nodemask) { return NULL; }
-static inline void free_gigantic_page(struct page *page, unsigned int order) { }
+static inline void free_gigantic_page(struct page *page, struct hstate *h) { }
static inline void destroy_compound_gigantic_page(struct page *page,
unsigned int order) { }
#endif
@@ -1172,7 +1097,7 @@ static void update_and_free_page(struct hstate *h, struct page *page)
set_page_refcounted(page);
if (hstate_is_gigantic(h)) {
destroy_compound_gigantic_page(page, huge_page_order(h));
- free_gigantic_page(page, huge_page_order(h));
+ free_gigantic_page(page, h);
} else {
__free_pages(page, huge_page_order(h));
}
--
2.13.6
^ permalink raw reply related
* [PATCH 2/3] mm: add find_alloc_contig_pages() interface
From: Mike Kravetz @ 2018-04-17 2:09 UTC (permalink / raw)
To: linux-mm, linux-kernel, linux-api
Cc: Reinette Chatre, Michal Hocko, Christopher Lameter, Guy Shattah,
Anshuman Khandual, Michal Nazarewicz, Vlastimil Babka,
David Nellans, Laura Abbott, Pavel Machek, Dave Hansen,
Andrew Morton, Mike Kravetz
In-Reply-To: <20180417020915.11786-1-mike.kravetz@oracle.com>
find_alloc_contig_pages() is a new interface that attempts to locate
and allocate a contiguous range of pages. It is provided as a more
convenient interface than alloc_contig_range() which is currently
used by CMA and gigantic huge pages.
When attempting to allocate a range of pages, migration is employed
if possible. There is no guarantee that the routine will succeed.
So, the user must be prepared for failure and have a fall back plan.
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
include/linux/gfp.h | 12 ++++++++
mm/page_alloc.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 99 insertions(+), 2 deletions(-)
diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 86a0d06463ab..528b194cc266 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -573,6 +573,18 @@ static inline bool pm_suspended_storage(void)
extern int alloc_contig_range(unsigned long start, unsigned long end,
unsigned migratetype, gfp_t gfp_mask);
extern void free_contig_range(unsigned long pfn, unsigned long nr_pages);
+extern struct page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
+ int nid, nodemask_t *nodemask);
+extern void free_contig_pages(struct page *page, unsigned long nr_pages);
+#else
+static inline page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
+ int nid, nodemask_t *nodemask)
+{
+ return NULL;
+}
+static void free_contig_pages(struct page *page, unsigned long nr_pages)
+{
+}
#endif
#ifdef CONFIG_CMA
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 0fd5e8e2456e..81070fe55c44 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -67,6 +67,7 @@
#include <linux/ftrace.h>
#include <linux/lockdep.h>
#include <linux/nmi.h>
+#include <linux/mmzone.h>
#include <asm/sections.h>
#include <asm/tlbflush.h>
@@ -2010,9 +2011,13 @@ static __always_inline struct page *__rmqueue_cma_fallback(struct zone *zone,
{
return __rmqueue_smallest(zone, order, MIGRATE_CMA);
}
+#define contig_alloc_migratetype_ok(migratetype) \
+ ((migratetype) == MIGRATE_CMA || (migratetype) == MIGRATE_MOVABLE)
#else
static inline struct page *__rmqueue_cma_fallback(struct zone *zone,
unsigned int order) { return NULL; }
+#define contig_alloc_migratetype_ok(migratetype) \
+ ((migratetype) == MIGRATE_MOVABLE)
#endif
/*
@@ -7822,6 +7827,9 @@ int alloc_contig_range(unsigned long start, unsigned long end,
};
INIT_LIST_HEAD(&cc.migratepages);
+ if (!contig_alloc_migratetype_ok(migratetype))
+ return -EINVAL;
+
/*
* What we do here is we mark all pageblocks in range as
* MIGRATE_ISOLATE. Because pageblock and max order pages may
@@ -7912,8 +7920,9 @@ int alloc_contig_range(unsigned long start, unsigned long end,
/* Make sure the range is really isolated. */
if (test_pages_isolated(outer_start, end, false)) {
- pr_info_ratelimited("%s: [%lx, %lx) PFNs busy\n",
- __func__, outer_start, end);
+ if (!(migratetype == MIGRATE_MOVABLE)) /* only print for CMA */
+ pr_info_ratelimited("%s: [%lx, %lx) PFNs busy\n",
+ __func__, outer_start, end);
ret = -EBUSY;
goto done;
}
@@ -7949,6 +7958,82 @@ void free_contig_range(unsigned long pfn, unsigned long nr_pages)
}
WARN(count != 0, "%ld pages are still in use!\n", count);
}
+
+static bool contig_pfn_range_valid(struct zone *z, unsigned long start_pfn,
+ unsigned long nr_pages)
+{
+ unsigned long i, end_pfn = start_pfn + nr_pages;
+ struct page *page;
+
+ for (i = start_pfn; i < end_pfn; i++) {
+ if (!pfn_valid(i))
+ return false;
+
+ page = pfn_to_page(i);
+
+ if (page_zone(page) != z)
+ return false;
+
+ }
+
+ return true;
+}
+
+/**
+ * find_alloc_contig_pages() -- attempt to find and allocate a contiguous
+ * range of pages
+ * @order: number of pages
+ * @gfp: gfp mask used to limit search as well as during compaction
+ * @nid: target node
+ * @nodemask: mask of other possible nodes
+ *
+ * Pages can be freed with a call to free_contig_pages(), or by manually
+ * calling __free_page() for each page allocated.
+ *
+ * Return: pointer to 'order' pages on success, or NULL if not successful.
+ */
+struct page *find_alloc_contig_pages(unsigned int order, gfp_t gfp,
+ int nid, nodemask_t *nodemask)
+{
+ unsigned long pfn, nr_pages, flags;
+ struct page *ret_page = NULL;
+ struct zonelist *zonelist;
+ struct zoneref *z;
+ struct zone *zone;
+ int rc;
+
+ nr_pages = 1 << order;
+ zonelist = node_zonelist(nid, gfp);
+ for_each_zone_zonelist_nodemask(zone, z, zonelist, gfp_zone(gfp),
+ nodemask) {
+ spin_lock_irqsave(&zone->lock, flags);
+ pfn = ALIGN(zone->zone_start_pfn, nr_pages);
+ while (zone_spans_pfn(zone, pfn + nr_pages - 1)) {
+ if (contig_pfn_range_valid(zone, pfn, nr_pages)) {
+ spin_unlock_irqrestore(&zone->lock, flags);
+
+ rc = alloc_contig_range(pfn, pfn + nr_pages,
+ MIGRATE_MOVABLE, gfp);
+ if (!rc) {
+ ret_page = pfn_to_page(pfn);
+ return ret_page;
+ }
+ spin_lock_irqsave(&zone->lock, flags);
+ }
+ pfn += nr_pages;
+ }
+ spin_unlock_irqrestore(&zone->lock, flags);
+ }
+
+ return ret_page;
+}
+EXPORT_SYMBOL_GPL(find_alloc_contig_pages);
+
+void free_contig_pages(struct page *page, unsigned long nr_pages)
+{
+ free_contig_range(page_to_pfn(page), nr_pages);
+}
+EXPORT_SYMBOL_GPL(free_contig_pages);
#endif
#if defined CONFIG_MEMORY_HOTPLUG || defined CONFIG_CMA
--
2.13.6
^ permalink raw reply related
* [PATCH 1/3] mm: change type of free_contig_range(nr_pages) to unsigned long
From: Mike Kravetz @ 2018-04-17 2:09 UTC (permalink / raw)
To: linux-mm, linux-kernel, linux-api
Cc: Reinette Chatre, Michal Hocko, Christopher Lameter, Guy Shattah,
Anshuman Khandual, Michal Nazarewicz, Vlastimil Babka,
David Nellans, Laura Abbott, Pavel Machek, Dave Hansen,
Andrew Morton, Mike Kravetz
In-Reply-To: <20180417020915.11786-1-mike.kravetz@oracle.com>
free_contig_range() is currently defined as:
void free_contig_range(unsigned long pfn, unsigned nr_pages);
change to,
void free_contig_range(unsigned long pfn, unsigned long nr_pages);
Some callers are passing a truncated unsigned long today. It is
highly unlikely that these values will overflow an unsigned int.
However, this should be changed to an unsigned long to be consistent
with other page counts.
Signed-off-by: Mike Kravetz <mike.kravetz@oracle.com>
---
include/linux/gfp.h | 2 +-
mm/cma.c | 2 +-
mm/hugetlb.c | 2 +-
mm/page_alloc.c | 6 +++---
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 1a4582b44d32..86a0d06463ab 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -572,7 +572,7 @@ static inline bool pm_suspended_storage(void)
/* The below functions must be run on a range from a single zone. */
extern int alloc_contig_range(unsigned long start, unsigned long end,
unsigned migratetype, gfp_t gfp_mask);
-extern void free_contig_range(unsigned long pfn, unsigned nr_pages);
+extern void free_contig_range(unsigned long pfn, unsigned long nr_pages);
#endif
#ifdef CONFIG_CMA
diff --git a/mm/cma.c b/mm/cma.c
index aa40e6c7b042..f473fc2b7cbd 100644
--- a/mm/cma.c
+++ b/mm/cma.c
@@ -563,7 +563,7 @@ bool cma_release(struct cma *cma, const struct page *pages, unsigned int count)
VM_BUG_ON(pfn + count > cma->base_pfn + cma->count);
- free_contig_range(pfn, count);
+ free_contig_range(pfn, (unsigned long)count);
cma_clear_bitmap(cma, pfn, count);
trace_cma_release(pfn, pages, count);
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 218679138255..c81072ce7510 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1055,7 +1055,7 @@ static void destroy_compound_gigantic_page(struct page *page,
static void free_gigantic_page(struct page *page, unsigned int order)
{
- free_contig_range(page_to_pfn(page), 1 << order);
+ free_contig_range(page_to_pfn(page), 1UL << order);
}
static int __alloc_gigantic_page(unsigned long start_pfn,
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 905db9d7962f..0fd5e8e2456e 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -7937,9 +7937,9 @@ int alloc_contig_range(unsigned long start, unsigned long end,
return ret;
}
-void free_contig_range(unsigned long pfn, unsigned nr_pages)
+void free_contig_range(unsigned long pfn, unsigned long nr_pages)
{
- unsigned int count = 0;
+ unsigned long count = 0;
for (; nr_pages--; pfn++) {
struct page *page = pfn_to_page(pfn);
@@ -7947,7 +7947,7 @@ void free_contig_range(unsigned long pfn, unsigned nr_pages)
count += page_count(page) != 1;
__free_page(page);
}
- WARN(count != 0, "%d pages are still in use!\n", count);
+ WARN(count != 0, "%ld pages are still in use!\n", count);
}
#endif
--
2.13.6
^ permalink raw reply related
* [PATCH 0/3] Interface for higher order contiguous allocations
From: Mike Kravetz @ 2018-04-17 2:09 UTC (permalink / raw)
To: linux-mm, linux-kernel, linux-api
Cc: Reinette Chatre, Michal Hocko, Christopher Lameter, Guy Shattah,
Anshuman Khandual, Michal Nazarewicz, Vlastimil Babka,
David Nellans, Laura Abbott, Pavel Machek, Dave Hansen,
Andrew Morton, Mike Kravetz
These patches came out of the "[RFC] mmap(MAP_CONTIG)" discussions at:
http://lkml.kernel.org/r/21f1ec96-2822-1189-1c95-79a2bb491571@oracle.com
One suggestion in that thread was to create a friendlier interface that
could be used by drivers and others outside core mm code to allocate a
contiguous set of pages. The alloc_contig_range() interface is used for
this purpose today by CMA and gigantic page allocation. However, this is
not a general purpose interface. So, wrap alloc_contig_range() in the
more general interface:
struct page *find_alloc_contig_pages(unsigned int order, gfp_t gfp, int nid,
nodemask_t *nodemask)
This interface is essentially the same functionality provided by the
hugetlb specific routine alloc_gigantic_page(). After creating the
interface, change alloc_gigantic_page() to call find_alloc_contig_pages()
and delete all the supporting code in hugetlb.c.
A new use case for allocating contiguous memory has been identified in
Intel(R) Resource Director Technology Cache Pseudo-Locking.
Mike Kravetz (3):
mm: change type of free_contig_range(nr_pages) to unsigned long
mm: add find_alloc_contig_pages() interface
mm/hugetlb: use find_alloc_contig_pages() to allocate gigantic pages
include/linux/gfp.h | 14 +++++++-
mm/cma.c | 2 +-
mm/hugetlb.c | 87 ++++--------------------------------------------
mm/page_alloc.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++---
4 files changed, 110 insertions(+), 88 deletions(-)
--
2.13.6
^ permalink raw reply
* Re: [PATCH] mmap.2: MAP_FIXED is okay if the address range has been reserved
From: Jann Horn @ 2018-04-16 21:12 UTC (permalink / raw)
To: Michal Hocko
Cc: Michael Kerrisk (man-pages), John Hubbard, linux-man,
Andrew Morton, Linux-MM, lkml, Linux API
In-Reply-To: <20180416211115.GU17484@dhcp22.suse.cz>
On Mon, Apr 16, 2018 at 11:11 PM, Michal Hocko <mhocko@kernel.org> wrote:
> On Mon 16-04-18 22:17:40, Jann Horn wrote:
>> On Mon, Apr 16, 2018 at 9:57 PM, Michal Hocko <mhocko@kernel.org> wrote:
>> > On Mon 16-04-18 21:30:09, Jann Horn wrote:
>> >> On Mon, Apr 16, 2018 at 9:18 PM, Michal Hocko <mhocko@kernel.org> wrote:
>> > [...]
>> >> > Yes, reasonably well written application will not have this problem.
>> >> > That, however, requires an external synchronization and that's why
>> >> > called it error prone and racy. I guess that was the main motivation for
>> >> > that part of the man page.
>> >>
>> >> What requires external synchronization? I still don't understand at
>> >> all what you're talking about.
>> >>
>> >> The following code:
>> >>
>> >> void *try_to_alloc_addr(void *hint, size_t len) {
>> >> char *x = mmap(hint, len, ...);
>> >> if (x == MAP_FAILED) return NULL;
>> >> if (x == hint) return x;
>> >
>> > Any other thread can modify the address space at this moment.
>>
>> But not parts of the address space that were returned by this mmap() call.
> ?
>> > Just
>> > consider that another thread would does mmap(x, MAP_FIXED) (or any other
>> > address overlapping [x, x+len] range)
>>
>> If the other thread does that without previously having created a
>> mapping covering the area in question, that would be a bug in the
>> other thread.
>
> MAP_FIXED is sometimes used without preallocated address ranges.
Wow, really? Can you point to an example?
>> MAP_FIXED on an unmapped address is almost always a bug
>> (excluding single-threaded cases with no library code, and even then
>> it's quite weird) - for example, any malloc() call could also cause
>> libc to start using the memory range you're trying to map with
>> MAP_FIXED.
>
> Yeah and that's why we there is such a large paragraph in the man page
> ;)
>
>> > becaus it is seemingly safe as x
>> > != hint.
>>
>> I don't understand this part. Are you talking about a hypothetical
>> scenario in which a programmer attempts to segment the virtual memory
>> space into areas that are exclusively used by threads without creating
>> memory mappings for those areas?
>
> Yeah, that doesn't sound all that over-exaggerated, right? And yes,
> such a code would be subtle and most probably buggy. I am not trying to
> argue for those hypothetical cases. All I am saying is that MAP_FIXED is
> subtle.
>
> I _do_ agree that using it solely on the preallocated and _properly_
> managed address ranges is safe. I still maintain my position on error
> prone though. And besides that there are usecases which do not operate
> on preallocated address ranges so people really have to be careful.
>
> I do not really care what is the form. I find the current wording quite
> informative and showing examples of how things might be broken. I do
> agree with your remark that "MAP_FIXED on preallocated ranges is safe"
> should be added. But MAP_FIXED is dangerous API and should have few big
> fat warnings.
> --
> Michal Hocko
> SUSE Labs
^ permalink raw reply
* Re: [PATCH] mmap.2: MAP_FIXED is okay if the address range has been reserved
From: Michal Hocko @ 2018-04-16 21:11 UTC (permalink / raw)
To: Jann Horn
Cc: Michael Kerrisk (man-pages), John Hubbard, linux-man,
Andrew Morton, Linux-MM, lkml, Linux API
In-Reply-To: <CAG48ez1bV_zZP3Y2ioDndP+H8mLCcxOtU1vCbWe7Q8myEGfXQQ@mail.gmail.com>
On Mon 16-04-18 22:17:40, Jann Horn wrote:
> On Mon, Apr 16, 2018 at 9:57 PM, Michal Hocko <mhocko@kernel.org> wrote:
> > On Mon 16-04-18 21:30:09, Jann Horn wrote:
> >> On Mon, Apr 16, 2018 at 9:18 PM, Michal Hocko <mhocko@kernel.org> wrote:
> > [...]
> >> > Yes, reasonably well written application will not have this problem.
> >> > That, however, requires an external synchronization and that's why
> >> > called it error prone and racy. I guess that was the main motivation for
> >> > that part of the man page.
> >>
> >> What requires external synchronization? I still don't understand at
> >> all what you're talking about.
> >>
> >> The following code:
> >>
> >> void *try_to_alloc_addr(void *hint, size_t len) {
> >> char *x = mmap(hint, len, ...);
> >> if (x == MAP_FAILED) return NULL;
> >> if (x == hint) return x;
> >
> > Any other thread can modify the address space at this moment.
>
> But not parts of the address space that were returned by this mmap() call.
?
> > Just
> > consider that another thread would does mmap(x, MAP_FIXED) (or any other
> > address overlapping [x, x+len] range)
>
> If the other thread does that without previously having created a
> mapping covering the area in question, that would be a bug in the
> other thread.
MAP_FIXED is sometimes used without preallocated address ranges.
> MAP_FIXED on an unmapped address is almost always a bug
> (excluding single-threaded cases with no library code, and even then
> it's quite weird) - for example, any malloc() call could also cause
> libc to start using the memory range you're trying to map with
> MAP_FIXED.
Yeah and that's why we there is such a large paragraph in the man page
;)
> > becaus it is seemingly safe as x
> > != hint.
>
> I don't understand this part. Are you talking about a hypothetical
> scenario in which a programmer attempts to segment the virtual memory
> space into areas that are exclusively used by threads without creating
> memory mappings for those areas?
Yeah, that doesn't sound all that over-exaggerated, right? And yes,
such a code would be subtle and most probably buggy. I am not trying to
argue for those hypothetical cases. All I am saying is that MAP_FIXED is
subtle.
I _do_ agree that using it solely on the preallocated and _properly_
managed address ranges is safe. I still maintain my position on error
prone though. And besides that there are usecases which do not operate
on preallocated address ranges so people really have to be careful.
I do not really care what is the form. I find the current wording quite
informative and showing examples of how things might be broken. I do
agree with your remark that "MAP_FIXED on preallocated ranges is safe"
should be added. But MAP_FIXED is dangerous API and should have few big
fat warnings.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH for 4.18 12/23] cpu_opv: Provide cpu_opv system call (v7)
From: Mathieu Desnoyers @ 2018-04-16 20:58 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Peter Zijlstra, Paul E. McKenney, Boqun Feng,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Russell King, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Andrew Hunter, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon
In-Reply-To: <CA+55aFxtZXi05UNLmtbKcZ=2N_ROE5pLnu9+v6FaXiuK=VbErA@mail.gmail.com>
----- On Apr 16, 2018, at 3:26 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Mon, Apr 16, 2018 at 12:21 PM, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> And I try very hard to avoid being told I'm the one breaking
>> user-space. ;-)
>
> You *can't* be breaking user space. User space doesn't use this yet.
>
> That's actually why I'd like to start with the minimal set - to make
> sure we don't introduce features that will come back to bite us later.
>
> The one compelling use case I saw was a memory allocator that used
> this for getting per-CPU (vs per-thread) memory scaling.
>
> That code didn't need the cpu_opv system call at all.
>
> And if somebody does a ldload of a malloc library, and then wants to
> analyze the behavior of a program, maybe they should ldload their own
> malloc routines first? That's pretty much par for the course for those
> kinds of projects.
>
> So I'd much rather we first merge the non-contentious parts that
> actually have some numbers for "this improves performance and makes a
> nice fancy malloc possible".
>
> As it is, the cpu_opv seems to be all about theory, not about actual need.
I fully get your point about getting the minimal feature in. So let's focus
on rseq only.
I will rework the patchset so the rseq selftests don't depend on cpu_opv,
and remove the cpu_opv stuff. I think it would be a good start for the
Facebook guys (jemalloc), given that just rseq seems to be enough for them
for now. It should be enough for the arm64 performance counters as well.
Then we'll figure out what is needed to make other projects use it based on
their needs (e.g. lttng-ust, liburcu, glibc malloc), and whether jemalloc
end up requiring cpu_opv for memory migration between per-cpu pools after all.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH] mmap.2: MAP_FIXED is okay if the address range has been reserved
From: Jann Horn @ 2018-04-16 20:17 UTC (permalink / raw)
To: Michal Hocko
Cc: Michael Kerrisk (man-pages), John Hubbard, linux-man,
Andrew Morton, Linux-MM, lkml, Linux API
In-Reply-To: <20180416195726.GT17484@dhcp22.suse.cz>
On Mon, Apr 16, 2018 at 9:57 PM, Michal Hocko <mhocko@kernel.org> wrote:
> On Mon 16-04-18 21:30:09, Jann Horn wrote:
>> On Mon, Apr 16, 2018 at 9:18 PM, Michal Hocko <mhocko@kernel.org> wrote:
> [...]
>> > Yes, reasonably well written application will not have this problem.
>> > That, however, requires an external synchronization and that's why
>> > called it error prone and racy. I guess that was the main motivation for
>> > that part of the man page.
>>
>> What requires external synchronization? I still don't understand at
>> all what you're talking about.
>>
>> The following code:
>>
>> void *try_to_alloc_addr(void *hint, size_t len) {
>> char *x = mmap(hint, len, ...);
>> if (x == MAP_FAILED) return NULL;
>> if (x == hint) return x;
>
> Any other thread can modify the address space at this moment.
But not parts of the address space that were returned by this mmap() call.
> Just
> consider that another thread would does mmap(x, MAP_FIXED) (or any other
> address overlapping [x, x+len] range)
If the other thread does that without previously having created a
mapping covering the area in question, that would be a bug in the
other thread. MAP_FIXED on an unmapped address is almost always a bug
(excluding single-threaded cases with no library code, and even then
it's quite weird) - for example, any malloc() call could also cause
libc to start using the memory range you're trying to map with
MAP_FIXED.
> becaus it is seemingly safe as x
> != hint.
I don't understand this part. Are you talking about a hypothetical
scenario in which a programmer attempts to segment the virtual memory
space into areas that are exclusively used by threads without creating
memory mappings for those areas?
> This will succeed and ...
>> munmap(x, len);
> ... now you are munmaping somebody's else memory range
>
>> return NULL;
>
> Do code _is_ buggy but it is not obvious at all.
>
>> }
>>
>> has no need for any form of external synchronization.
>
> If the above mmap/munmap section was protected by a lock and _all_ other
> mmaps (direct or indirect) would use the same lock then you are safe
> against that.
^ permalink raw reply
* Re: [PATCH] mmap.2: MAP_FIXED is okay if the address range has been reserved
From: Michal Hocko @ 2018-04-16 19:57 UTC (permalink / raw)
To: Jann Horn
Cc: Michael Kerrisk (man-pages), John Hubbard, linux-man,
Andrew Morton, Linux-MM, lkml, Linux API
In-Reply-To: <CAG48ez1nf96nHj8a+aZy22RwqYTUZBGrsGFcz=ZhZBUWzaEZ9w@mail.gmail.com>
On Mon 16-04-18 21:30:09, Jann Horn wrote:
> On Mon, Apr 16, 2018 at 9:18 PM, Michal Hocko <mhocko@kernel.org> wrote:
[...]
> > Yes, reasonably well written application will not have this problem.
> > That, however, requires an external synchronization and that's why
> > called it error prone and racy. I guess that was the main motivation for
> > that part of the man page.
>
> What requires external synchronization? I still don't understand at
> all what you're talking about.
>
> The following code:
>
> void *try_to_alloc_addr(void *hint, size_t len) {
> char *x = mmap(hint, len, ...);
> if (x == MAP_FAILED) return NULL;
> if (x == hint) return x;
Any other thread can modify the address space at this moment. Just
consider that another thread would does mmap(x, MAP_FIXED) (or any other
address overlapping [x, x+len] range) becaus it is seemingly safe as x
!= hint. This will succeed and ...
> munmap(x, len);
... now you are munmaping somebody's else memory range
> return NULL;
Do code _is_ buggy but it is not obvious at all.
> }
>
> has no need for any form of external synchronization.
If the above mmap/munmap section was protected by a lock and _all_ other
mmaps (direct or indirect) would use the same lock then you are safe
against that.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] mmap.2: MAP_FIXED is okay if the address range has been reserved
From: Jann Horn @ 2018-04-16 19:30 UTC (permalink / raw)
To: Michal Hocko
Cc: Michael Kerrisk (man-pages), John Hubbard, linux-man,
Andrew Morton, Linux-MM, lkml, Linux API
In-Reply-To: <20180416191805.GS17484@dhcp22.suse.cz>
On Mon, Apr 16, 2018 at 9:18 PM, Michal Hocko <mhocko@kernel.org> wrote:
> On Mon 16-04-18 15:55:36, Jann Horn wrote:
>> On Mon, Apr 16, 2018 at 12:07 PM, Michal Hocko <mhocko@kernel.org> wrote:
>> > On Fri 13-04-18 18:17:36, Jann Horn wrote:
>> >> On Fri, Apr 13, 2018 at 6:05 PM, Jann Horn <jannh@google.com> wrote:
>> >> > On Fri, Apr 13, 2018 at 6:04 PM, Michal Hocko <mhocko@kernel.org> wrote:
>> >> >> On Fri 13-04-18 17:04:09, Jann Horn wrote:
>> >> >>> On Fri, Apr 13, 2018 at 8:49 AM, Michal Hocko <mhocko@kernel.org> wrote:
>> >> >>> > On Fri 13-04-18 08:43:27, Michael Kerrisk wrote:
>> >> >>> > [...]
>> >> >>> >> So, you mean remove this entire paragraph:
>> >> >>> >>
>> >> >>> >> For cases in which the specified memory region has not been
>> >> >>> >> reserved using an existing mapping, newer kernels (Linux
>> >> >>> >> 4.17 and later) provide an option MAP_FIXED_NOREPLACE that
>> >> >>> >> should be used instead; older kernels require the caller to
>> >> >>> >> use addr as a hint (without MAP_FIXED) and take appropriate
>> >> >>> >> action if the kernel places the new mapping at a different
>> >> >>> >> address.
>> >> >>> >>
>> >> >>> >> It seems like some version of the first half of the paragraph is worth
>> >> >>> >> keeping, though, so as to point the reader in the direction of a remedy.
>> >> >>> >> How about replacing that text with the following:
>> >> >>> >>
>> >> >>> >> Since Linux 4.17, the MAP_FIXED_NOREPLACE flag can be used
>> >> >>> >> in a multithreaded program to avoid the hazard described
>> >> >>> >> above.
>> >> >>> >
>> >> >>> > Yes, that sounds reasonable to me.
>> >> >>>
>> >> >>> But that kind of sounds as if you can't avoid it before Linux 4.17,
>> >> >>> when actually, you just have to call mmap() with the address as hint,
>> >> >>> and if mmap() returns a different address, munmap() it and go on your
>> >> >>> normal error path.
>> >> >>
>> >> >> This is still racy in multithreaded application which is the main point
>> >> >> of the whole section, no?
>> >> >
>> >> > No, it isn't.
>> >
>> > I could have been more specific, sorry.
>> >
>> >> mmap() with a hint (without MAP_FIXED) will always non-racily allocate
>> >> a memory region for you or return an error code. If it does allocate a
>> >> memory region, it belongs to you until you deallocate it. It might be
>> >> at a different address than you requested -
>> >
>> > Yes, this all is true. Except the atomicity is guaranteed only for the
>> > syscall. Once you return to the userspace any error handling is error
>> > prone and racy because your mapping might change under you feet. So...
>>
>> Can you please elaborate on why you think anything could change the
>> mapping returned by mmap() under the caller's feet?
>
> Because as soon as the mmap_sem is dropped then any other thread can
> modify the shared address space.
>
>> When mmap() returns a memory area to the caller, that memory area
>> belongs to the caller. No unrelated code will touch it, unless that
>> code is buggy.
>
> Yes, reasonably well written application will not have this problem.
> That, however, requires an external synchronization and that's why
> called it error prone and racy. I guess that was the main motivation for
> that part of the man page.
What requires external synchronization? I still don't understand at
all what you're talking about.
The following code:
void *try_to_alloc_addr(void *hint, size_t len) {
char *x = mmap(hint, len, ...);
if (x == MAP_FAILED) return NULL;
if (x == hint) return x;
munmap(x, len);
return NULL;
}
has no need for any form of external synchronization. You can call it
in library code, you can call it in a multithreaded process, you can
call it wherever and it should be safe.
mmap() atomically reserves previously unallocated memory, and nothing
else should be touching that memory until it is released again using
munmap(). (Just like malloc(): When you call malloc(), you get a chunk
of memory that is reserved just for you, and nobody else will scribble
over it until you call free().)
^ permalink raw reply
* Re: [RFC PATCH for 4.18 12/23] cpu_opv: Provide cpu_opv system call (v7)
From: Linus Torvalds @ 2018-04-16 19:26 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Andy Lutomirski, Peter Zijlstra, Paul E. McKenney, Boqun Feng,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Russell King, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Andrew Hunter, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon
In-Reply-To: <435471300.11403.1523906479091.JavaMail.zimbra@efficios.com>
On Mon, Apr 16, 2018 at 12:21 PM, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
> And I try very hard to avoid being told I'm the one breaking
> user-space. ;-)
You *can't* be breaking user space. User space doesn't use this yet.
That's actually why I'd like to start with the minimal set - to make
sure we don't introduce features that will come back to bite us later.
The one compelling use case I saw was a memory allocator that used
this for getting per-CPU (vs per-thread) memory scaling.
That code didn't need the cpu_opv system call at all.
And if somebody does a ldload of a malloc library, and then wants to
analyze the behavior of a program, maybe they should ldload their own
malloc routines first? That's pretty much par for the course for those
kinds of projects.
So I'd much rather we first merge the non-contentious parts that
actually have some numbers for "this improves performance and makes a
nice fancy malloc possible".
As it is, the cpu_opv seems to be all about theory, not about actual need.
Linus
^ permalink raw reply
* Re: [RFC PATCH for 4.18 12/23] cpu_opv: Provide cpu_opv system call (v7)
From: Mathieu Desnoyers @ 2018-04-16 19:21 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andy Lutomirski, Peter Zijlstra, Paul E. McKenney, Boqun Feng,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Russell King, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
Andrew Hunter, Andi Kleen, Chris Lameter, Ben Maurer, rostedt,
Josh Triplett, Catalin Marinas, Will Deacon
In-Reply-To: <CA+55aFzZtY5ELKc+TGf9BA6px300BNzedadm_yNXzODTKN=HgQ@mail.gmail.com>
----- On Apr 16, 2018, at 2:39 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
> On Mon, Apr 16, 2018 at 11:35 AM, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>> Specifically for single-stepping, the __rseq_table section introduced
>> at user-level will allow newer debuggers and tools which do line and
>> instruction-level single-stepping to skip over rseq critical sections.
>> However, this breaks existing debuggers and tools.
>
> I really don't think single-stepping is a valid argument.
>
> Even if the cpu_opv() allows you to "single step", you're not actually
> single stepping the same thing that you're using. So you are literally
> debugging something else than the real code.
>
> At that point, you don't need "cpu_opv()", you need to just load
> /dev/urandom in a buffer, and single-step that. Ta-daa! No new kernel
> functionality needed.
>
> So if the main argument for cpu_opv is single-stepping, then just rip
> it out. It's not useful.
No, single-stepping is not the only use-case. Accessing remote cpu
data is another use-case fulfilled by cpu_opv, which I think is more
compelling.
>
> Anybody who cares deeply about single-stepping shouldn't be using
> optimistic algorithms, and they shouldn't be doing multi-threaded
> stuff either. They won't be able to use things like transactional
> memory either.
>
> You can't single-step into the kernel to see what the kernel does
> either when you're debugging something.
>
> News at 11: "single stepping isn't always viable".
I don't mind if people cannot stop the program with a debugger and
observe the state of registers manually at each step though a rseq
critical section.
I do mind breaking existing tools that rely on single-stepping
approaches to automatically analyze program behavior [1,2].
Introducing a rseq critical section into a library (e.g. glibc
memory allocator) would cause existing programs being analyzed
with existing tools to hang.
And I try very hard to avoid being told I'm the one breaking
user-space. ;-)
Thanks,
Mathieu
[1] http://rr-project.org/
[2] https://www.gnu.org/software/gdb/news/reversible.html
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ 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