Linux userland API discussions
 help / color / mirror / Atom feed

* Re: [PATCH v27 04/12] LRNG - add switchable DRNG support
From: Stephan Müller @ 2020-01-12 10:12 UTC (permalink / raw)
  To: kbuild test robot
  Cc: kbuild-all, Arnd Bergmann, Greg Kroah-Hartman, linux-crypto, LKML,
	linux-api, Eric W. Biederman, Alexander E. Patrakov,
	Ahmed S. Darwish, Theodore Y. Ts'o, Willy Tarreau,
	Matthew Garrett, Vito Caputo, Andreas Dilger, Jan Kara,
	Ray Strode, William Jon McCann, zhangjs, Andy Lutomirski,
	Florian Weimer, Lennart Poettering, Ni
In-Reply-To: <202001111540.5y2emdGQ%lkp@intel.com>

Am Samstag, 11. Januar 2020, 08:09:50 CET schrieb kbuild test robot:

Hi,

> Hi "Stephan,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on char-misc/char-misc-testing]
> [also build test WARNING on cryptodev/master crypto/master v5.5-rc5
> next-20200110] [if your patch is applied to the wrong git tree, please drop
> us a note to help improve the system. BTW, we also suggest to use '--base'
> option to specify the base tree in git format-patch, please see
> https://stackoverflow.com/a/37406982]
> 
> url:   
> https://github.com/0day-ci/linux/commits/Stephan-M-ller/dev-random-a-new-ap
> proach-with-full-SP800-90B/20200110-084934 base:  
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
> 68faa679b8be1a74e6663c21c3a9d25d32f1c079 reproduce:
>         # apt-get install sparse
>         # sparse version: v0.6.1-129-g341daf20-dirty
>         make ARCH=x86_64 allmodconfig
>         make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
> 
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
> 
> 
> sparse warnings: (new ones prefixed by >>)
> 
> >> drivers/char/lrng/lrng_internal.h:239:39: sparse: sparse: context
> >> imbalance in 'lrng_drng_switch' - unexpected unlock
> vim +/lrng_drng_switch +239 drivers/char/lrng/lrng_internal.h
> 
> 58c283819a1e87 Stephan Müller 2020-01-09  233
> 58c283819a1e87 Stephan Müller 2020-01-09  234  /* Unlock the DRNG */
> 58c283819a1e87 Stephan Müller 2020-01-09  235  static __always_inline void
> lrng_drng_unlock(struct lrng_drng *drng, 58c283819a1e87 Stephan Müller
> 2020-01-09  236  					     unsigned long *flags) 
58c283819a1e87 Stephan
> Müller 2020-01-09  237  {
> 58c283819a1e87 Stephan Müller 2020-01-09  238  	if
> (lrng_drng_is_atomic(drng)) 58c283819a1e87 Stephan Müller 2020-01-09 @239 
> 		spin_unlock_irqrestore(&drng->spin_lock, *flags); 58c283819a1e87 
Stephan
> Müller 2020-01-09  240  	else
> 58c283819a1e87 Stephan Müller 2020-01-09  241  		mutex_unlock(&drng-
>lock);
> 58c283819a1e87 Stephan Müller 2020-01-09  242  }
> 58c283819a1e87 Stephan Müller 2020-01-09  243
> 
> :::::: The code at line 239 was first introduced by commit
> :::::: 58c283819a1e879bc2e30d05720285f9709f7f6d Linux Random Number
> :::::: Generator
> :::::: 
> :::::: TO: Stephan Müller <smueller@chronox.de>
> :::::: CC: 0day robot <lkp@intel.com>
> 
> ---
> 0-DAY kernel test infrastructure                 Open Source Technology
> Center https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel
> Corporation

After analyzing the issue a bit more, it seems that I have to remove 
"unlikely" from lrng_drng_lock which seems to cause additional grief
with sparse. Note, sparse will still report a lock context imbalance as it
used to since we indeed have two lock context as documented in
lrng_drng_switch.

Ciao
Stephan

^ permalink raw reply

* Re: [PATCHv8 20/34] lib/vdso: Prepare for time namespace support
From: Thomas Gleixner @ 2020-01-12 10:32 UTC (permalink / raw)
  To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: Dmitry Safonov, Dmitry Safonov, Adrian Reber, Andrei Vagin,
	Andy Lutomirski, Arnd Bergmann, Christian Brauner,
	Cyrill Gorcunov, Eric W. Biederman, H. Peter Anvin, Ingo Molnar,
	Jann Horn, Jeff Dike, Oleg Nesterov, Pavel Emelyanov, Shuah Khan,
	Vincenzo Frascino,
	containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	criu-GEFAQzZX7r8dnm+yROfE0A, linux-api-u79uwXL29TY76Z2rM5mHXA,
	x86-DgEjT+Ai2ygdnm+yROfE0A, Andrei Vagin
In-Reply-To: <20191112012724.250792-21-dima-nzgTgzXrdUbQT0dZR+AlfA@public.gmane.org>

Dmitry Safonov <dima-nzgTgzXrdUbQT0dZR+AlfA@public.gmane.org> writes:
> From: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> +#ifdef CONFIG_TIME_NS
> +static int do_hres_timens(const struct vdso_data *vdns, clockid_t clk,
> +		      struct __kernel_timespec *ts)
> +{
> +	const struct vdso_data *vd = __arch_get_timens_vdso_data();
> +	const struct vdso_timestamp *vdso_ts;
> +	const struct timens_offset *offs = &vdns->offset[clk];
> +	u64 cycles, last, ns;
> +	u32 seq, msk;
> +	s64 sec;
> +
> +	msk = 1U << clk;
> +	if (msk & VDSO_HRES)
> +		vd = &vd[CS_HRES_COARSE];
> +	else if (msk & VDSO_RAW)
> +		vd = &vd[CS_RAW];
> +	else
> +		return -1;

This part is redundant. The initial call site already made sure that
this is only called for clocks matching VDSO_HRES or VDSO_RAW. I just
drop it.

Thanks,

        tglx

^ permalink raw reply

* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Al Viro @ 2020-01-12 21:33 UTC (permalink / raw)
  To: Ian Kent
  Cc: Linus Torvalds, Aleksa Sarai, David Howells, Eric Biederman,
	stable, Christian Brauner, Serge Hallyn,
	dev-IGmTWi+3HBZvNhPySn5qfx2eb7JE58TQ, Linux Containers, Linux API,
	linux-fsdevel, Linux Kernel Mailing List
In-Reply-To: <979cf680b0fbdce515293a3449d564690cde6a3f.camel-PKsaG3nR2I+sTnJN9+BGXg@public.gmane.org>

On Fri, Jan 10, 2020 at 02:20:55PM +0800, Ian Kent wrote:

> Yeah, autofs ->d_automount() doesn't return -EISDIR, by the time
> we get there it's not relevant any more, so that check looks
> redundant. I'm not aware of any other fs automount implementation
> that needs that EISDIR pass-thru function.
> 
> I didn't notice it at the time of the merge, sorry about that.
> 
> While we're at it that:
>    if (!path->dentry->d_op || !path->dentry->d_op->d_automount)
>        return -EREMOTE;
> 
> at the top of follow_automount() isn't going to be be relevant
> for autofs because ->d_automount() really must always be defined
> for it.
> 
> But, at the time of the merge, I didn't object to it because
> there were (are) other file systems that use the VFS automount
> function which may accidentally not define the method.

OK...

> > Unfortunately, there are other interesting questions related to
> > autofs-specific bits (->d_manage()) and the timezone-related fun
> > is, of course, still there.  I hope to sort that out today or
> > tomorrow, at least enough to do a reasonable set of backportable
> > fixes to put in front of follow_managed()/step_into() queue.
> > Oh, well...
> 
> Yeah, I know it slows you down but I kink-off like having a chance

Nice typo, that ;-)

> to look at what's going and think about your questions before trying
> to answer them, rather than replying prematurely, as I usually do ...
> 
> It's been a bit of a busy day so far but I'm getting to look into
> the questions you've asked.

Here's a bit more of those (I might've missed some of your replies on
IRC; my apologies if that's the case):

1) AFAICS, -EISDIR from ->d_manage() actually means "don't even try
->d_automount() here".  If its effect can be delayed until the decision
to call ->d_automount(), the things seem to get simpler.  Is it ever
returned in situation when the sucker _is_ overmounted?

2) can autofs_d_automount() ever be called for a daemon?  Looks like it
shouldn't be...

3) is _anything_ besides root directory ever created in direct autofs
superblocks by anyone?  If not, why does autofs_lookup() even bother to
do anything there?  IOW, why not have it return ERR_PTR(-ENOENT) immediately
for direct ones?  Or am I missing something and it is, in fact, possible
to have the daemon create something in those?

4) Symlinks look like they should qualify for parent being non-empty;
at least autofs_d_manage() seems to think so (simple_empty() use).
So shouldn't we remove the trap from its parent on symlink/restore on
unlink if parent gets empty?  For version 4 or earlier, that is.  Or is
it simply that daemon only creates symlinks in root directory?


Anyway, intermediate state of the series is in #work.namei right now,
and some _very_ interesting possibilities open up.  It definitely
needs more massage around __follow_mount_rcu() (as it is, the
fastpath in there is still too twisted).  Said that
	* call graph is less convoluted
	* follow_managed() calls are folded into step_into().  Interface:
int step_into(nd, flags, dentry, inode, seq), with inode/seq used only
if we are in RCU mode.
	* ".." still doesn't use that; it probably ought to.
	* lookup_fast() doesn't take path - nd, &inode, &seq and returns dentry
	* lookup_open() and fs/namei.c:atomic_open() get similar treatment
- don't take path, return dentry.
	* calls of follow_managed()/step_into() combination returning 1
are always followed by get_link(), and very shortly, at that.  So much
that we can realistically merge pick_link() (in the end of
step_into()) with get_link().  That merge is NOT done in this branch yet.

The last one promises to get rid of a rather unpleasant group of calling
conventions.  Right now we have several functions (step_into()/
walk_component()/lookup_last()/do_last()) with the following calling
conventions:
	-E...	=> error
	0	=> non-symlink or symlink not followed; nd->path points to it
	1	=> picked a symlink to follow; its mount/dentry/seq has been
pushed on nd->stack[]; its inode is stashed into nd->link_inode for
subsequent get_link() to pick.  nd->path is left unchanged.

That way all of those become
	ERR_PTR(-E...)	=> error
	NULL		=> non-symlink, symlink not followed or a pure
jump (bare "/" or procfs ones); nd->path points to where we end up
        string		=> symlink being followed; the sucker's pushed
to stack, initial jump (if any) has been handled and the string returned
is what we need to traverse.

IMO it's less arbitrary that way.  More importantly, the separation between
step_into() committing to symlink traversal and (inevitably following)
get_link() is gone - it's one operation after that change.  No nd->link_inode
either - it's only needed to carry the information from pick_link() to the
next get_link().

Loops turn into
	while (!(err = link_path_walk(nd, s)) &&
	       (s = lookup_last(nd)) != NULL)
		;
and
	while (!(err = link_path_walk(nd, s)) &&
	       (s = do_last(nd, file, op)) != NULL)
		;

trailing_symlink() goes away (folded into pick_link()/get_link() combo,
conditional upon nd->depth at the entry).  And in link_path_walk() we'll
have
                if (unlikely(!*name)) {
                        /* pathname body, done */
                        if (!nd->depth)
                                return 0;
                        name = nd->stack[nd->depth - 1].name;
                        /* trailing symlink, done */
                        if (!name)
                                return 0;
                        /* last component of nested symlink */
                        s = walk_component(nd, WALK_FOLLOW);
                } else {
                        /* not the last component */
                        s = walk_component(nd, WALK_FOLLOW | WALK_MORE);
                }
                if (s) {
                        if (IS_ERR(s))
                                return PTR_ERR(s);
			/* a symlink to follow */
			nd->stack[nd->depth - 1].name = name;
                        name = s;
                        continue;
                }

Anyway, before I try that one I'm going to fold path_openat2() into
that series - that step is definitely going to require some massage
there; it's too close to get_link() changes done in Aleksa's series.

If we do that, we get a single primitive for "here's the result of
lookup; traverse mounts and either move into the result or, if
it's a symlink that needs to be traversed, start the symlink
traversal - jump into the base position for it (if needed) and
return the pathname that needs to be handled".  As it is, mainline
has that logics spread over about a dozen locations...

Diffstat at the moment:
 fs/autofs/dev-ioctl.c |   6 +-
 fs/internal.h         |   1 -
 fs/namei.c            | 460 ++++++++++++++------------------------------------
 fs/namespace.c        |  97 +++++++----
 fs/nfs/nfstrace.h     |   2 -
 fs/open.c             |   4 +-
 include/linux/namei.h |   3 +-
 7 files changed, 197 insertions(+), 376 deletions(-)

In the current form the sucker appears to work (so far - about 30%
into the usual xfstests run) without visible slowdowns...

^ permalink raw reply

* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Ian Kent @ 2020-01-13  1:48 UTC (permalink / raw)
  To: Al Viro, Aleksa Sarai
  Cc: David Howells, Eric Biederman, Linus Torvalds, stable,
	Christian Brauner, Serge Hallyn, dev, containers, linux-api,
	linux-fsdevel, linux-kernel
In-Reply-To: <20200110231945.GL8904@ZenIV.linux.org.uk>

On Fri, 2020-01-10 at 23:19 +0000, Al Viro wrote:
> On Fri, Jan 03, 2020 at 01:49:01AM +0000, Al Viro wrote:
> > On Thu, Jan 02, 2020 at 02:59:20PM +1100, Aleksa Sarai wrote:
> > > On 2020-01-01, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > > > On Thu, Jan 02, 2020 at 01:44:07AM +1100, Aleksa Sarai wrote:
> > > > 
> > > > > Thanks, this fixes the issue for me (and also fixes another
> > > > > reproducer I
> > > > > found -- mounting a symlink on top of itself then trying to
> > > > > umount it).
> > > > > 
> > > > > Reported-by: Aleksa Sarai <cyphar@cyphar.com>
> > > > > Tested-by: Aleksa Sarai <cyphar@cyphar.com>
> > > > 
> > > > Pushed into #fixes.
> > > 
> > > Thanks. One other thing I noticed is that umount applies to the
> > > underlying symlink rather than the mountpoint on top. So, for
> > > example
> > > (using the same scripts I posted in the thread):
> > > 
> > >   # ln -s /tmp/foo link
> > >   # ./mount_to_symlink /etc/passwd link
> > >   # umount -l link # will attempt to unmount "/tmp/foo"
> > > 
> > > Is that intentional?
> > 
> > It's a mess, again in mountpoint_last().  FWIW, at some point I
> > proposed
> > to have nd_jump_link() to fail with -ELOOP if the target was a
> > symlink;
> > Linus asked for reasons deeper than my dislike of the semantics, I
> > looked
> > around and hadn't spotted anything.  And there hadn't been at the
> > time,
> > but when four months later umount_lookup_last() went in I failed to
> > look
> > for that source of potential problems in it ;-/
> 
> FWIW, since Ian appears to agree that we want ->d_manage() on the
> mount
> crossing at the end of umount(2) lookup, here's a much simpler
> solution -
> kill mountpoint_last() and switch to using lookup_last().  As a side
> benefit, LOOKUP_NO_REVAL also goes away.  It's possible to trim the
> things even more (path_mountpoint() is very similar to
> path_lookupat()
> at that point, and it's not hard to make the differences conditional
> on
> something like LOOKUP_UMOUNT); I would rather do that part in the
> cleanups series - the one below is easier to backport.
> 
> Aleksa, Ian - could you see if the patch below works for you?

I did try this patch and I was trying to work out why it didn't
work. But thought I'd let you know what I saw.

Applying it to current Linus tree systemd stops at switch root.

Not sure what causes that, I couldn't see any reason for it.

I see you have a development branch in your repo. I'll have a look
at that rather than continue with this.

> 
> commit e56b43b971a7c08762fceab330a52b7245041dbc
> Author: Al Viro <viro@zeniv.linux.org.uk>
> Date:   Fri Jan 10 17:17:19 2020 -0500
> 
>     reimplement path_mountpoint() with less magic
>     
>     ... and get rid of a bunch of bugs in it.  Background:
>     the reason for path_mountpoint() is that umount() really doesn't
>     want attempts to revalidate the root of what it's trying to
> umount.
>     The thing we want to avoid actually happen from complete_walk();
>     solution was to do something parallel to normal path_lookupat()
>     and it both went overboard and got the boilerplate subtly
>     (and not so subtly) wrong.
>     
>     A better solution is to do pretty much what the normal
> path_lookupat()
>     does, but instead of complete_walk() do unlazy_walk().  All it
> takes
>     to avoid that ->d_weak_revalidate() call...  mountpoint_last()
> goes
>     away, along with everything it got wrong, and so does the magic
> around
>     LOOKUP_NO_REVAL.
>     
>     Another source of bugs is that when we traverse mounts at the
> final
>     location (and we need to do that - umount . expects to get
> whatever's
>     overmounting ., if any, out of the lookup) we really ought to
> take
>     care of ->d_manage() - as it is, manual umount of autofs
> automount
>     in progress can lead to unpleasant surprises for the
> daemon.  Easily
>     solved by using handle_lookup_down() instead of follow_mount().
>     
>     Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index d6c91d1e88cb..1793661c3342 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1649,17 +1649,15 @@ static struct dentry *__lookup_slow(const
> struct qstr *name,
>  	if (IS_ERR(dentry))
>  		return dentry;
>  	if (unlikely(!d_in_lookup(dentry))) {
> -		if (!(flags & LOOKUP_NO_REVAL)) {
> -			int error = d_revalidate(dentry, flags);
> -			if (unlikely(error <= 0)) {
> -				if (!error) {
> -					d_invalidate(dentry);
> -					dput(dentry);
> -					goto again;
> -				}
> +		int error = d_revalidate(dentry, flags);
> +		if (unlikely(error <= 0)) {
> +			if (!error) {
> +				d_invalidate(dentry);
>  				dput(dentry);
> -				dentry = ERR_PTR(error);
> +				goto again;
>  			}
> +			dput(dentry);
> +			dentry = ERR_PTR(error);
>  		}
>  	} else {
>  		old = inode->i_op->lookup(inode, dentry, flags);
> @@ -2618,72 +2616,6 @@ int user_path_at_empty(int dfd, const char
> __user *name, unsigned flags,
>  EXPORT_SYMBOL(user_path_at_empty);
>  
>  /**
> - * mountpoint_last - look up last component for umount
> - * @nd:   pathwalk nameidata - currently pointing at parent
> directory of "last"
> - *
> - * This is a special lookup_last function just for umount. In this
> case, we
> - * need to resolve the path without doing any revalidation.
> - *
> - * The nameidata should be the result of doing a LOOKUP_PARENT
> pathwalk. Since
> - * mountpoints are always pinned in the dcache, their ancestors are
> too. Thus,
> - * in almost all cases, this lookup will be served out of the
> dcache. The only
> - * cases where it won't are if nd->last refers to a symlink or the
> path is
> - * bogus and it doesn't exist.
> - *
> - * Returns:
> - * -error: if there was an error during lookup. This includes
> -ENOENT if the
> - *         lookup found a negative dentry.
> - *
> - * 0:      if we successfully resolved nd->last and found it to not
> to be a
> - *         symlink that needs to be followed.
> - *
> - * 1:      if we successfully resolved nd->last and found it to be a
> symlink
> - *         that needs to be followed.
> - */
> -static int
> -mountpoint_last(struct nameidata *nd)
> -{
> -	int error = 0;
> -	struct dentry *dir = nd->path.dentry;
> -	struct path path;
> -
> -	/* If we're in rcuwalk, drop out of it to handle last component
> */
> -	if (nd->flags & LOOKUP_RCU) {
> -		if (unlazy_walk(nd))
> -			return -ECHILD;
> -	}
> -
> -	nd->flags &= ~LOOKUP_PARENT;
> -
> -	if (unlikely(nd->last_type != LAST_NORM)) {
> -		error = handle_dots(nd, nd->last_type);
> -		if (error)
> -			return error;
> -		path.dentry = dget(nd->path.dentry);
> -	} else {
> -		path.dentry = d_lookup(dir, &nd->last);
> -		if (!path.dentry) {
> -			/*
> -			 * No cached dentry. Mounted dentries are
> pinned in the
> -			 * cache, so that means that this dentry is
> probably
> -			 * a symlink or the path doesn't actually point
> -			 * to a mounted dentry.
> -			 */
> -			path.dentry = lookup_slow(&nd->last, dir,
> -					     nd->flags |
> LOOKUP_NO_REVAL);
> -			if (IS_ERR(path.dentry))
> -				return PTR_ERR(path.dentry);
> -		}
> -	}
> -	if (d_flags_negative(smp_load_acquire(&path.dentry->d_flags)))
> {
> -		dput(path.dentry);
> -		return -ENOENT;
> -	}
> -	path.mnt = nd->path.mnt;
> -	return step_into(nd, &path, 0, d_backing_inode(path.dentry),
> 0);
> -}
> -
> -/**
>   * path_mountpoint - look up a path to be umounted
>   * @nd:		lookup context
>   * @flags:	lookup flags
> @@ -2699,14 +2631,17 @@ path_mountpoint(struct nameidata *nd,
> unsigned flags, struct path *path)
>  	int err;
>  
>  	while (!(err = link_path_walk(s, nd)) &&
> -		(err = mountpoint_last(nd)) > 0) {
> +		(err = lookup_last(nd)) > 0) {
>  		s = trailing_symlink(nd);
>  	}
> +	if (!err)
> +		err = unlazy_walk(nd);
> +	if (!err)
> +		err = handle_lookup_down(nd);
>  	if (!err) {
>  		*path = nd->path;
>  		nd->path.mnt = NULL;
>  		nd->path.dentry = NULL;
> -		follow_mount(path);
>  	}
>  	terminate_walk(nd);
>  	return err;
> diff --git a/fs/nfs/nfstrace.h b/fs/nfs/nfstrace.h
> index f64a33d2a1d1..2a82dcce5fc1 100644
> --- a/fs/nfs/nfstrace.h
> +++ b/fs/nfs/nfstrace.h
> @@ -206,7 +206,6 @@ TRACE_DEFINE_ENUM(LOOKUP_AUTOMOUNT);
>  TRACE_DEFINE_ENUM(LOOKUP_PARENT);
>  TRACE_DEFINE_ENUM(LOOKUP_REVAL);
>  TRACE_DEFINE_ENUM(LOOKUP_RCU);
> -TRACE_DEFINE_ENUM(LOOKUP_NO_REVAL);
>  TRACE_DEFINE_ENUM(LOOKUP_OPEN);
>  TRACE_DEFINE_ENUM(LOOKUP_CREATE);
>  TRACE_DEFINE_ENUM(LOOKUP_EXCL);
> @@ -224,7 +223,6 @@ TRACE_DEFINE_ENUM(LOOKUP_DOWN);
>  			{ LOOKUP_PARENT, "PARENT" }, \
>  			{ LOOKUP_REVAL, "REVAL" }, \
>  			{ LOOKUP_RCU, "RCU" }, \
> -			{ LOOKUP_NO_REVAL, "NO_REVAL" }, \
>  			{ LOOKUP_OPEN, "OPEN" }, \
>  			{ LOOKUP_CREATE, "CREATE" }, \
>  			{ LOOKUP_EXCL, "EXCL" }, \
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 7fe7b87a3ded..07bfb0874033 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -34,7 +34,6 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT,
> LAST_BIND};
>  
>  /* internal use only */
>  #define LOOKUP_PARENT		0x0010
> -#define LOOKUP_NO_REVAL		0x0080
>  #define LOOKUP_JUMPED		0x1000
>  #define LOOKUP_ROOT		0x2000
>  #define LOOKUP_ROOT_GRABBED	0x0008

^ permalink raw reply

* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Ian Kent @ 2020-01-13  2:59 UTC (permalink / raw)
  To: Al Viro
  Cc: Linus Torvalds, Aleksa Sarai, David Howells, Eric Biederman,
	stable, Christian Brauner, Serge Hallyn, dev, Linux Containers,
	Linux API, linux-fsdevel, Linux Kernel Mailing List
In-Reply-To: <20200112213352.GP8904@ZenIV.linux.org.uk>

On Sun, 2020-01-12 at 21:33 +0000, Al Viro wrote:
> On Fri, Jan 10, 2020 at 02:20:55PM +0800, Ian Kent wrote:
> 
> > Yeah, autofs ->d_automount() doesn't return -EISDIR, by the time
> > we get there it's not relevant any more, so that check looks
> > redundant. I'm not aware of any other fs automount implementation
> > that needs that EISDIR pass-thru function.
> > 
> > I didn't notice it at the time of the merge, sorry about that.
> > 
> > While we're at it that:
> >    if (!path->dentry->d_op || !path->dentry->d_op->d_automount)
> >        return -EREMOTE;
> > 
> > at the top of follow_automount() isn't going to be be relevant
> > for autofs because ->d_automount() really must always be defined
> > for it.
> > 
> > But, at the time of the merge, I didn't object to it because
> > there were (are) other file systems that use the VFS automount
> > function which may accidentally not define the method.
> 
> OK...
> 
> > > Unfortunately, there are other interesting questions related to
> > > autofs-specific bits (->d_manage()) and the timezone-related fun
> > > is, of course, still there.  I hope to sort that out today or
> > > tomorrow, at least enough to do a reasonable set of backportable
> > > fixes to put in front of follow_managed()/step_into() queue.
> > > Oh, well...
> > 
> > Yeah, I know it slows you down but I kink-off like having a chance
> 
> Nice typo, that ;-)
> 
> > to look at what's going and think about your questions before
> > trying
> > to answer them, rather than replying prematurely, as I usually do
> > ...
> > 
> > It's been a bit of a busy day so far but I'm getting to look into
> > the questions you've asked.
> 
> Here's a bit more of those (I might've missed some of your replies on
> IRC; my apologies if that's the case):
> 
> 1) AFAICS, -EISDIR from ->d_manage() actually means "don't even try
> ->d_automount() here".  If its effect can be delayed until the
> decision
> to call ->d_automount(), the things seem to get simpler.  Is it ever
> returned in situation when the sucker _is_ overmounted?

In theory it shouldn't need to be returned when there is an
actual mount there.

If there is a real mount at this point that should be enough to
prevent walks into that mount until it's mount is complete.

The whole idea of -EISDIR is to prevent processes from walking
into a directory tree that "doesn't have a real mount at its
base" (the so called multi-mount map construct).

> 
> 2) can autofs_d_automount() ever be called for a daemon?  Looks like
> it
> shouldn't be...

Can't do that, it will lead to deadlock very quickly.

> 
> 3) is _anything_ besides root directory ever created in direct autofs
> superblocks by anyone?  If not, why does autofs_lookup() even bother
> to
> do anything there?  IOW, why not have it return ERR_PTR(-ENOENT)
> immediately
> for direct ones?  Or am I missing something and it is, in fact,
> possible
> to have the daemon create something in those?

Short answer is no, longer answer is directories "shouldn't" ever
be created inside direct mount points.

The thing is that the multi-mount map construct can be used with
direct mounts too, but they must always have a real mount at the
base because they are direct mounts. So processes should not be
able to walk into them while they are being mounted (constructed).

But I'm pretty sure it's rare (maybe not done at all) that this
map construct is used with direct mounts.

> 
> 4) Symlinks look like they should qualify for parent being non-empty;
> at least autofs_d_manage() seems to think so (simple_empty() use).
> So shouldn't we remove the trap from its parent on symlink/restore on
> unlink if parent gets empty?  For version 4 or earlier, that is.  Or
> is
> it simply that daemon only creates symlinks in root directory?

Yes, they have to be empty.

If a symlink is to be used (based on autofs config or map option)
and the "browse" option is used for the indirect mount (browse
only makes sense for indirect autofs managed mounts) then the
mount point directory has to be removed and a symlink created
so it must be empty to for this to make sense.

If it's a "nobrowse" autofs mount then nothing should already
exist, it just gets created.

The catch is that a map entry for which a symlink is to be used
instead of a mount can't be a multi-mount. I'm pretty sure I don't
have sufficient error checking for that in the daemon but I also
haven't had reports of problems with it either.

For a very long time the use of symlinks was not common but when
the amd format map parser was added it made sense to use symlinks
in some cases for those. That was partly to reduce the number of
mounts needed and because I deliberately don't support amd map
entries that provide the multi-mount construct. The way amd did
this looked ugly to me, very much a hack to add a Sun format
mount feature.

As far as keeping the trap flags up to date, I don't.

It seemed so much simpler to just leave the flags in place but,
at that time, symlinks were not used (although it was possible to
do so), now that's changed fiddling with the flags might now make
sense.

As I said on IRC:
"DCACHE_NEED_AUTOMOUNT is set on symlink dentries because, when
->lookup() is called the dentry may trigger a callback to the
daemon that will either create a directory (since, in this case,
one does not already exist) and attempt to mount on it or create
a symlink if the autofs config/map requires it.

I didn't think there would be potential simplification by setting
and clearing the DCACHE_NEED_AUTOMOUNT flag based on it being a
directory (mountpoint) or a symlink so the flag is always left set.
Although, as you point out, symlinks won't actually trigger mounts
so the flag being left set when the dentry is a symlink is due to
lazyness, since there's nothing to gain. If you can see potential
simplification in the VFS code by managing this flag better then
that would be worth while."

> 
> 
> Anyway, intermediate state of the series is in #work.namei right now,
> and some _very_ interesting possibilities open up.  It definitely
> needs more massage around __follow_mount_rcu() (as it is, the
> fastpath in there is still too twisted).  Said that
> 	* call graph is less convoluted
> 	* follow_managed() calls are folded into
> step_into().  Interface:
> int step_into(nd, flags, dentry, inode, seq), with inode/seq used
> only
> if we are in RCU mode.
> 	* ".." still doesn't use that; it probably ought to.
> 	* lookup_fast() doesn't take path - nd, &inode, &seq and
> returns dentry
> 	* lookup_open() and fs/namei.c:atomic_open() get similar
> treatment
> - don't take path, return dentry.
> 	* calls of follow_managed()/step_into() combination returning 1
> are always followed by get_link(), and very shortly, at that.  So
> much
> that we can realistically merge pick_link() (in the end of
> step_into()) with get_link().  That merge is NOT done in this branch
> yet.
> 
> The last one promises to get rid of a rather unpleasant group of
> calling
> conventions.  Right now we have several functions (step_into()/
> walk_component()/lookup_last()/do_last()) with the following calling
> conventions:
> 	-E...	=> error
> 	0	=> non-symlink or symlink not followed; nd->path points to it
> 	1	=> picked a symlink to follow; its mount/dentry/seq has been
> pushed on nd->stack[]; its inode is stashed into nd->link_inode for
> subsequent get_link() to pick.  nd->path is left unchanged.
> 
> That way all of those become
> 	ERR_PTR(-E...)	=> error
> 	NULL		=> non-symlink, symlink not followed or a
> pure
> jump (bare "/" or procfs ones); nd->path points to where we end up
>         string		=> symlink being followed; the sucker's
> pushed
> to stack, initial jump (if any) has been handled and the string
> returned
> is what we need to traverse.
> 
> IMO it's less arbitrary that way.  More importantly, the separation
> between
> step_into() committing to symlink traversal and (inevitably
> following)
> get_link() is gone - it's one operation after that change.  No nd-
> >link_inode
> either - it's only needed to carry the information from pick_link()
> to the
> next get_link().
> 
> Loops turn into
> 	while (!(err = link_path_walk(nd, s)) &&
> 	       (s = lookup_last(nd)) != NULL)
> 		;
> and
> 	while (!(err = link_path_walk(nd, s)) &&
> 	       (s = do_last(nd, file, op)) != NULL)
> 		;
> 
> trailing_symlink() goes away (folded into pick_link()/get_link()
> combo,
> conditional upon nd->depth at the entry).  And in link_path_walk()
> we'll
> have
>                 if (unlikely(!*name)) {
>                         /* pathname body, done */
>                         if (!nd->depth)
>                                 return 0;
>                         name = nd->stack[nd->depth - 1].name;
>                         /* trailing symlink, done */
>                         if (!name)
>                                 return 0;
>                         /* last component of nested symlink */
>                         s = walk_component(nd, WALK_FOLLOW);
>                 } else {
>                         /* not the last component */
>                         s = walk_component(nd, WALK_FOLLOW |
> WALK_MORE);
>                 }
>                 if (s) {
>                         if (IS_ERR(s))
>                                 return PTR_ERR(s);
> 			/* a symlink to follow */
> 			nd->stack[nd->depth - 1].name = name;
>                         name = s;
>                         continue;
>                 }
> 
> Anyway, before I try that one I'm going to fold path_openat2() into
> that series - that step is definitely going to require some massage
> there; it's too close to get_link() changes done in Aleksa's series.
> 
> If we do that, we get a single primitive for "here's the result of
> lookup; traverse mounts and either move into the result or, if
> it's a symlink that needs to be traversed, start the symlink
> traversal - jump into the base position for it (if needed) and
> return the pathname that needs to be handled".  As it is, mainline
> has that logics spread over about a dozen locations...
> 
> Diffstat at the moment:
>  fs/autofs/dev-ioctl.c |   6 +-
>  fs/internal.h         |   1 -
>  fs/namei.c            | 460 ++++++++++++++------------------------
> ------------
>  fs/namespace.c        |  97 +++++++----
>  fs/nfs/nfstrace.h     |   2 -
>  fs/open.c             |   4 +-
>  include/linux/namei.h |   3 +-
>  7 files changed, 197 insertions(+), 376 deletions(-)
> 
> In the current form the sucker appears to work (so far - about 30%
> into the usual xfstests run) without visible slowdowns...

Ok, I'll have a look at that branch, ;)

Ian

^ permalink raw reply

* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Al Viro @ 2020-01-13  3:54 UTC (permalink / raw)
  To: Ian Kent
  Cc: Aleksa Sarai, David Howells, Eric Biederman, Linus Torvalds,
	stable, Christian Brauner, Serge Hallyn, dev, containers,
	linux-api, linux-fsdevel, linux-kernel
In-Reply-To: <aea0bc800b6a1e547ca1944738ff9db4379098ba.camel@themaw.net>

On Mon, Jan 13, 2020 at 09:48:23AM +0800, Ian Kent wrote:

> I did try this patch and I was trying to work out why it didn't
> work. But thought I'd let you know what I saw.
> 
> Applying it to current Linus tree systemd stops at switch root.
> 
> Not sure what causes that, I couldn't see any reason for it.

Wait a minute...  So you are seeing problems early in the boot,
before any autofs ioctls might come into play?

Sigh...  Guess I'll have to dig that Fedora KVM image out and
try to see what it's about... ;-/  Here comes a couple of hours
of build...

^ permalink raw reply

* Re: [PATCHv8 02/34] lib/vdso: make do_hres and do_coarse as __always_inline
From: Andrei Vagin @ 2020-01-13  5:27 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Vincenzo Frascino, Dmitry Safonov, LKML, Dmitry Safonov,
	Adrian Reber, Andy Lutomirski, Arnd Bergmann, Christian Brauner,
	Cyrill Gorcunov, Eric W. Biederman, H. Peter Anvin, Ingo Molnar,
	Jann Horn, Jeff Dike, Oleg Nesterov, Pavel Emelyanov, Shuah Khan,
	Linux Containers, crml, Linux API, X86 ML
In-Reply-To: <875zhja59q.fsf@nanos.tec.linutronix.de>

On Fri, Jan 10, 2020 at 4:02 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Vincenzo Frascino <vincenzo.frascino@arm.com> writes:
> > On 1/10/20 11:42 AM, Thomas Gleixner wrote:
> >>>> +» » /*·goto·allows·to·avoid·extra·inlining·of·do_hres.·*/
> >>>> +» » goto·out_hres;
> >>>
> >>> What is the performance impact of "goto out_hres"?
> >>
> >> On x86 it's invisible at least in my limited testing.
> >
> > On arm64 as well based on mine as well. Shall we keep the code more readable
> > here (without goto)?
>
> The delta patch below makes it readable again and also avoids the double
> inlining. Quick testing shows no difference.

My tests show no difference too and the code looks more readable. Thanks!

>
> Thanks,
>
>         tglx

^ permalink raw reply

* I wait for your swift response,
From: Rick Schaech @ 2020-01-13  5:52 UTC (permalink / raw)


Dear, I'm Mr Rick Schaech, I am the General Account Auditor, Though i
know we have not meet each other before but sometimes in life God have
a reason of bringing two people from two different countries together
as business partners or life partners.

My dear friend, I have the sum of 15.7 Million USD i wish to put in
your name due to the death of my late client who died several years
ago as his next of kin column still remain blank. Though the internet
medium is highly abuse these days but am assuring you that this
transaction is legitimate and I am contacting you that we may have a
deal, note for your cooperation and collaboration 40% of the sum will
be for you while the other 60% will be for me as well. I wait for your
swift response for more details. please forward your response to my
personal E-mail: rickschaech-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

Yours sincerely,
Rick Schaech.

^ permalink raw reply

* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Ian Kent @ 2020-01-13  6:00 UTC (permalink / raw)
  To: Al Viro
  Cc: Aleksa Sarai, David Howells, Eric Biederman, Linus Torvalds,
	stable, Christian Brauner, Serge Hallyn, dev, containers,
	linux-api, linux-fsdevel, linux-kernel
In-Reply-To: <20200113035407.GQ8904@ZenIV.linux.org.uk>

On Mon, 2020-01-13 at 03:54 +0000, Al Viro wrote:
> On Mon, Jan 13, 2020 at 09:48:23AM +0800, Ian Kent wrote:
> 
> > I did try this patch and I was trying to work out why it didn't
> > work. But thought I'd let you know what I saw.
> > 
> > Applying it to current Linus tree systemd stops at switch root.
> > 
> > Not sure what causes that, I couldn't see any reason for it.
> 
> Wait a minute...  So you are seeing problems early in the boot,
> before any autofs ioctls might come into play?

I did, then I checked it booted without the patch, then tried
building from scratch with the patch twice and same thing
happened each time.

Looked like this, such as it is:
[ OK ] Reached target Switch Root.
[ OK ] Started Plymouth switch root service.
       Starting Switch Root...

I don't have any evidence but thought it might be this:
https://github.com/karelzak/util-linux/blob/master/sys-utils/switch_root.c

Mind you, that's not the actual systemd repo. either I probably
need to look a lot deeper (and at the actual systemd repo) to
work out what's actually being called.

> 
> Sigh...  Guess I'll have to dig that Fedora KVM image out and
> try to see what it's about... ;-/  Here comes a couple of hours
> of build...

^ permalink raw reply

* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Ian Kent @ 2020-01-13  6:03 UTC (permalink / raw)
  To: Al Viro
  Cc: Aleksa Sarai, David Howells, Eric Biederman, Linus Torvalds,
	stable, Christian Brauner, Serge Hallyn, dev, containers,
	linux-api, linux-fsdevel, linux-kernel
In-Reply-To: <41c535d689530f3715f21cd25074eb61e825a5f6.camel@themaw.net>

On Mon, 2020-01-13 at 14:00 +0800, Ian Kent wrote:
> On Mon, 2020-01-13 at 03:54 +0000, Al Viro wrote:
> > On Mon, Jan 13, 2020 at 09:48:23AM +0800, Ian Kent wrote:
> > 
> > > I did try this patch and I was trying to work out why it didn't
> > > work. But thought I'd let you know what I saw.
> > > 
> > > Applying it to current Linus tree systemd stops at switch root.
> > > 
> > > Not sure what causes that, I couldn't see any reason for it.
> > 
> > Wait a minute...  So you are seeing problems early in the boot,
> > before any autofs ioctls might come into play?
> 
> I did, then I checked it booted without the patch, then tried
> building from scratch with the patch twice and same thing
> happened each time.
> 
> Looked like this, such as it is:
> [ OK ] Reached target Switch Root.
> [ OK ] Started Plymouth switch root service.
>        Starting Switch Root...
> 
> I don't have any evidence but thought it might be this:
> https://github.com/karelzak/util-linux/blob/master/sys-utils/switch_root.c

Oh wait, for systemd I was actually looking at:
https://github.com/systemd/systemd/blob/master/src/shared/switch-root.c

> 
> Mind you, that's not the actual systemd repo. either I probably
> need to look a lot deeper (and at the actual systemd repo) to
> work out what's actually being called.
> 
> > Sigh...  Guess I'll have to dig that Fedora KVM image out and
> > try to see what it's about... ;-/  Here comes a couple of hours
> > of build...

^ permalink raw reply

* Re: [PATCH 2/4] mm: introduce external memory hinting API
From: Kirill Tkhai @ 2020-01-13  8:47 UTC (permalink / raw)
  To: Minchan Kim, Andrew Morton
  Cc: LKML, linux-mm, linux-api, oleksandr, Suren Baghdasaryan,
	Tim Murray, Daniel Colascione, Sandeep Patil, Sonny Rao,
	Brian Geffon, Michal Hocko, Johannes Weiner, Shakeel Butt,
	John Dias
In-Reply-To: <20200110213433.94739-3-minchan@kernel.org>

On 11.01.2020 00:34, Minchan Kim wrote:
> There are usecases that System Management Software(SMS) want to give
> a memory hint to other processes because it's not known to the
> application. In the case of Android, ActivityManagerService daemon
> manges app's life cycle and that daemon must be able to initiate
> reclaim on its own without any app involvement.
> 
> To solve the issue, this patch introduces new syscall process_madvise(2).
> It uses pidfd of an external processs to give the hint.
> 
>  int process_madvise(int pidfd, void *addr, size_t length, int advise,
> 			unsigned long flag);
> 
> Since it could affect other process's address range, only privileged
> process(CAP_SYS_PTRACE) or something else(e.g., being the same UID)
> gives it the right to ptrace the process could use it successfully.
> The flag argument is reserved for future use if we need to extend the
> API.
> 
> Supporting all hints madvise has/will supported/support to process_madvise
> is rather risky. Because we are not sure all hints make sense from external
> process and implementation for the hint may rely on the caller being
> in the current context so it could be error-prone. Thus, I just limited
> hints as MADV_[COLD|PAGEOUT] in this patch.
> 
> If someone want to add other hints, we could hear hear the usecase and
> review it for each hint. It's more safe for maintainace rather than
> introducing a buggy syscall but hard to fix it later.
> 
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
>  arch/alpha/kernel/syscalls/syscall.tbl      |  1 +
>  arch/arm/tools/syscall.tbl                  |  1 +
>  arch/arm64/include/asm/unistd.h             |  2 +-
>  arch/arm64/include/asm/unistd32.h           |  2 +
>  arch/ia64/kernel/syscalls/syscall.tbl       |  1 +
>  arch/m68k/kernel/syscalls/syscall.tbl       |  1 +
>  arch/microblaze/kernel/syscalls/syscall.tbl |  1 +
>  arch/mips/kernel/syscalls/syscall_n32.tbl   |  1 +
>  arch/mips/kernel/syscalls/syscall_n64.tbl   |  1 +
>  arch/parisc/kernel/syscalls/syscall.tbl     |  1 +
>  arch/powerpc/kernel/syscalls/syscall.tbl    |  1 +
>  arch/s390/kernel/syscalls/syscall.tbl       |  1 +
>  arch/sh/kernel/syscalls/syscall.tbl         |  1 +
>  arch/sparc/kernel/syscalls/syscall.tbl      |  1 +
>  arch/x86/entry/syscalls/syscall_32.tbl      |  1 +
>  arch/x86/entry/syscalls/syscall_64.tbl      |  1 +
>  arch/xtensa/kernel/syscalls/syscall.tbl     |  1 +
>  include/linux/syscalls.h                    |  2 +
>  include/uapi/asm-generic/unistd.h           |  5 +-
>  kernel/sys_ni.c                             |  1 +
>  mm/madvise.c                                | 64 +++++++++++++++++++++
>  21 files changed, 89 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
> index e56950f23b49..776c61803315 100644
> --- a/arch/alpha/kernel/syscalls/syscall.tbl
> +++ b/arch/alpha/kernel/syscalls/syscall.tbl
> @@ -477,3 +477,4 @@
>  # 545 reserved for clone3
>  546	common	watch_devices			sys_watch_devices
>  547	common	openat2				sys_openat2
> +548	common	process_madvise			sys_process_madvise
> diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
> index 7fb2f4d59210..a43381542276 100644
> --- a/arch/arm/tools/syscall.tbl
> +++ b/arch/arm/tools/syscall.tbl
> @@ -451,3 +451,4 @@
>  435	common	clone3				sys_clone3
>  436	common	watch_devices			sys_watch_devices
>  437	common	openat2				sys_openat2
> +438	common	process_madvise			sys_process_madvise
> diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
> index 8aa00ccb0b96..b722e47377a5 100644
> --- a/arch/arm64/include/asm/unistd.h
> +++ b/arch/arm64/include/asm/unistd.h
> @@ -38,7 +38,7 @@
>  #define __ARM_NR_compat_set_tls		(__ARM_NR_COMPAT_BASE + 5)
>  #define __ARM_NR_COMPAT_END		(__ARM_NR_COMPAT_BASE + 0x800)
>  
> -#define __NR_compat_syscalls		438
> +#define __NR_compat_syscalls		439
>  #endif
>  
>  #define __ARCH_WANT_SYS_CLONE
> diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
> index 31f0ce25719e..5c82557d408f 100644
> --- a/arch/arm64/include/asm/unistd32.h
> +++ b/arch/arm64/include/asm/unistd32.h
> @@ -883,6 +883,8 @@ __SYSCALL(__NR_clone3, sys_clone3)
>  __SYSCALL(__NR_watch_devices, sys_watch_devices)
>  #define __NR_openat2 437
>  __SYSCALL(__NR_openat2, sys_openat2)
> +#define __NR_openat2 438
> +__SYSCALL(__NR_process_madvise, process_madvise)
>  
>  /*
>   * Please add new compat syscalls above this comment and update
> diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl
> index b9aa59931905..c156abc9a298 100644
> --- a/arch/ia64/kernel/syscalls/syscall.tbl
> +++ b/arch/ia64/kernel/syscalls/syscall.tbl
> @@ -358,3 +358,4 @@
>  # 435 reserved for clone3
>  436	common	watch_devices			sys_watch_devices
>  437	common	openat2				sys_openat2
> +438	common	process_madvise			sys_process_madvise
> diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
> index 868c1ef89d35..5b6034b6650f 100644
> --- a/arch/m68k/kernel/syscalls/syscall.tbl
> +++ b/arch/m68k/kernel/syscalls/syscall.tbl
> @@ -437,3 +437,4 @@
>  # 435 reserved for clone3
>  436	common	watch_devices			sys_watch_devices
>  437	common	openat2				sys_openat2
> +438	common	process_madvise			sys_process_madvise
> diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
> index 544b4cef18b3..4bef584af09c 100644
> --- a/arch/microblaze/kernel/syscalls/syscall.tbl
> +++ b/arch/microblaze/kernel/syscalls/syscall.tbl
> @@ -443,3 +443,4 @@
>  435	common	clone3				sys_clone3
>  436	common	watch_devices			sys_watch_devices
>  437	common	openat2				sys_openat2
> +438	common	process_madvise			sys_process_madvise
> diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
> index 05e8aee5dae7..94fbd0fcccce 100644
> --- a/arch/mips/kernel/syscalls/syscall_n32.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
> @@ -376,3 +376,4 @@
>  435	n32	clone3				__sys_clone3
>  436	n32	watch_devices			sys_watch_devices
>  437	n32	openat2				sys_openat2
> +437	n32	process_madivse			sys_process_madvise

438. And several places below has the same mistake.

> diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
> index 24d6c01328fb..4e6982c429d5 100644
> --- a/arch/mips/kernel/syscalls/syscall_n64.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
> @@ -352,3 +352,4 @@
>  435	n64	clone3				__sys_clone3
>  436	n64	watch_devices			sys_watch_devices
>  437	n64	openat2				sys_openat2
> +437	n64	process_madvise			sys_process_madvise
> diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
> index 4b5f77a4e1a2..3aa990caf9dc 100644
> --- a/arch/parisc/kernel/syscalls/syscall.tbl
> +++ b/arch/parisc/kernel/syscalls/syscall.tbl
> @@ -435,3 +435,4 @@
>  435	common	clone3				sys_clone3_wrapper
>  436	common	watch_devices			sys_watch_devices
>  437	common	openat2				sys_openat2
> +437	common	process_madvise			sys_process_madvise
> diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
> index 9716dc85a517..30e727a23f33 100644
> --- a/arch/powerpc/kernel/syscalls/syscall.tbl
> +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
> @@ -519,3 +519,4 @@
>  435	nospu	clone3				ppc_clone3
>  436	common	watch_devices			sys_watch_devices
>  437	common	openat2				sys_openat2
> +437	common	process_madvise			sys_process_madvise
> diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
> index 7da330f8b03e..75722e5ff496 100644
> --- a/arch/s390/kernel/syscalls/syscall.tbl
> +++ b/arch/s390/kernel/syscalls/syscall.tbl
> @@ -440,3 +440,4 @@
>  435  common	clone3			sys_clone3			sys_clone3
>  436  common	watch_devices		sys_watch_devices		sys_watch_devices
>  437  common	openat2			sys_openat2			sys_openat2
> +437  common	process_madvise		sys_process_madvise		sys_process_madvise
> diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
> index bb7e68e25337..7d7bc7befad3 100644
> --- a/arch/sh/kernel/syscalls/syscall.tbl
> +++ b/arch/sh/kernel/syscalls/syscall.tbl
> @@ -440,3 +440,4 @@
>  # 435 reserved for clone3
>  436	common	watch_devices			sys_watch_devices
>  437	common	openat2				sys_openat2
> +437	common	process_madvise			sys_process_madvise
> diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
> index 646a1fad7218..581d331ff62f 100644
> --- a/arch/sparc/kernel/syscalls/syscall.tbl
> +++ b/arch/sparc/kernel/syscalls/syscall.tbl
> @@ -483,3 +483,4 @@
>  # 435 reserved for clone3
>  436	common	watch_devices			sys_watch_devices
>  437	common	openat2			sys_openat2
> +437	common	process_madvise		sys_process_madvise
> diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> index 57c53acee290..76a2c266fe7e 100644
> --- a/arch/x86/entry/syscalls/syscall_32.tbl
> +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> @@ -442,3 +442,4 @@
>  435	i386	clone3			sys_clone3			__ia32_sys_clone3
>  436	i386	watch_devices		sys_watch_devices		__ia32_sys_watch_devices
>  437	i386	openat2			sys_openat2			__ia32_sys_openat2
> +438	i386	process_madvise		sys_process_madvise		__ia32_sys_process_madvise
> diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> index 1dd8d21f6500..b697cd8620cb 100644
> --- a/arch/x86/entry/syscalls/syscall_64.tbl
> +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> @@ -359,6 +359,7 @@
>  435	common	clone3			__x64_sys_clone3/ptregs
>  436	common	watch_devices		__x64_sys_watch_devices
>  437	common	openat2			__x64_sys_openat2
> +438	common	process_madvise		__x64_sys_process_madvise
>  
>  #
>  # x32-specific system call numbers start at 512 to avoid cache impact
> diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
> index 0f48ab7bd75b..2e9813ecfd7d 100644
> --- a/arch/xtensa/kernel/syscalls/syscall.tbl
> +++ b/arch/xtensa/kernel/syscalls/syscall.tbl
> @@ -408,3 +408,4 @@
>  435	common	clone3				sys_clone3
>  436	common	watch_devices			sys_watch_devices
>  437	common	openat2				sys_openat2
> +438	common	process_madvise			sys_process_madvise
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 433c8c85636e..1b58a11ff49f 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -877,6 +877,8 @@ asmlinkage long sys_munlockall(void);
>  asmlinkage long sys_mincore(unsigned long start, size_t len,
>  				unsigned char __user * vec);
>  asmlinkage long sys_madvise(unsigned long start, size_t len, int behavior);
> +asmlinkage long sys_process_madvise(int pidfd, unsigned long start,
> +			size_t len, int behavior, unsigned long flags);
>  asmlinkage long sys_remap_file_pages(unsigned long start, unsigned long size,
>  			unsigned long prot, unsigned long pgoff,
>  			unsigned long flags);
> diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> index 33f3856a9c3c..4bcd8d366f38 100644
> --- a/include/uapi/asm-generic/unistd.h
> +++ b/include/uapi/asm-generic/unistd.h
> @@ -856,8 +856,11 @@ __SYSCALL(__NR_watch_devices, sys_watch_devices)
>  #define __NR_openat2 437
>  __SYSCALL(__NR_openat2, sys_openat2)
>  
> +#define __NR_openat2 438
> +__SYSCALL(__NR_process_madvise, sys_process_madvise)
> +
>  #undef __NR_syscalls
> -#define __NR_syscalls 438
> +#define __NR_syscalls 439
>  
>  /*
>   * 32 bit systems traditionally used different
> diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
> index 0e9b275260f8..10ce5eac8b4b 100644
> --- a/kernel/sys_ni.c
> +++ b/kernel/sys_ni.c
> @@ -281,6 +281,7 @@ COND_SYSCALL(mlockall);
>  COND_SYSCALL(munlockall);
>  COND_SYSCALL(mincore);
>  COND_SYSCALL(madvise);
> +COND_SYSCALL(process_madvise);
>  COND_SYSCALL(remap_file_pages);
>  COND_SYSCALL(mbind);
>  COND_SYSCALL_COMPAT(mbind);
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 0c901de531e4..e15dfb4df7bf 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -17,6 +17,7 @@
>  #include <linux/falloc.h>
>  #include <linux/fadvise.h>
>  #include <linux/sched.h>
> +#include <linux/sched/mm.h>
>  #include <linux/ksm.h>
>  #include <linux/fs.h>
>  #include <linux/file.h>
> @@ -993,6 +994,18 @@ madvise_behavior_valid(int behavior)
>  	}
>  }
>  
> +static bool
> +process_madvise_behavior_valid(int behavior)
> +{
> +	switch (behavior) {
> +	case MADV_COLD:
> +	case MADV_PAGEOUT:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  /*
>   * madvise_common - request behavior hint to address range of the target process
>   *
> @@ -1169,3 +1182,54 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
>  {
>  	return madvise_common(current, current->mm, start, len_in, behavior);
>  }
> +
> +SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
> +		size_t, len_in, int, behavior, unsigned long, flags)

I don't like the interface. The fact we have pidfd does not mean,
we have to use it for new syscalls always. A user may want to set
madvise for specific pid from console and pass pid as argument.
pidfd would be an overkill in this case.
We usually call "kill -9 pid" from console. Why shouldn't process_madvise()
allow this?

I suggent to extend first argument to work with both pid and pidfd.
Look at what we have for waitid(idtype, id_t id, ...) for example:

       idtype == P_PID
              Wait for the child whose process ID matches id.

       idtype == P_PIDFD (since Linux 5.4)
              Wait for the child referred to by the PID file descriptor specified in id.  (See pidfd_open(2) for  further  information  on
              PID file descriptors.)

We may use @flags argument for this.

> +{
> +	int ret;
> +	struct fd f;
> +	struct pid *pid;
> +	struct task_struct *task;
> +	struct mm_struct *mm;
> +
> +	if (flags != 0)
> +		return -EINVAL;
> +
> +	if (!process_madvise_behavior_valid(behavior))
> +		return -EINVAL;
> +
> +	f = fdget(pidfd);
> +	if (!f.file)
> +		return -EBADF;
> +
> +	pid = pidfd_pid(f.file);
> +	if (IS_ERR(pid)) {
> +		ret = PTR_ERR(pid);
> +		goto err;
> +	}

>From here:

> +	rcu_read_lock();
> +	task = pid_task(pid, PIDTYPE_PID);
> +	if (!task) {
> +		rcu_read_unlock();
> +		ret = -ESRCH;
> +		goto err;
> +	}
> +
> +	get_task_struct(task);
> +	rcu_read_unlock();

to here your code is a copy of existing get_pid_task(). We should the primitive instead.

> +
> +	mm = mm_access(task, PTRACE_MODE_ATTACH_FSCREDS);
> +	if (IS_ERR_OR_NULL(mm)) {
> +		ret = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
> +		goto release_task;
> +	}
> +
> +	ret = madvise_common(task, mm, start, len_in, behavior);
> +	mmput(mm);
> +release_task:
> +	put_task_struct(task);
> +err:

Maybe s/err:/fdput:/ for uniformity with the above "release_task:"?

> +	fdput(f);
> +	return ret;
> +}
> 

^ permalink raw reply

* Re: [PATCH v27 12/12] LRNG - add power-on and runtime self-tests
From: Dan Carpenter @ 2020-01-13 10:39 UTC (permalink / raw)
  To: kbuild, Stephan Müller
  Cc: kbuild-all, Arnd Bergmann, Greg Kroah-Hartman, linux-crypto, LKML,
	linux-api, Eric W. Biederman, Alexander E. Patrakov,
	Ahmed S. Darwish, Theodore Y. Ts'o, Willy Tarreau,
	Matthew Garrett, Vito Caputo, Andreas Dilger, Jan Kara,
	Ray Strode, William Jon McCann, zhangjs, Andy Lutomirski,
	Florian Weimer, Lennart Poettering, Ni
In-Reply-To: <2355906.JbblJTOqSk@positron.chronox.de>

Hi "Stephan,

url:    https://github.com/0day-ci/linux/commits/Stephan-M-ller/dev-random-a-new-approach-with-full-SP800-90B/20200110-084934
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 68faa679b8be1a74e6663c21c3a9d25d32f1c079

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/char/lrng/lrng_selftest.c:117 lrng_pool_lfsr_selftest() warn: sizeof(NUMBER)?
drivers/char/lrng/lrng_selftest.c:229 lrng_hash_df_selftest() warn: sizeof(NUMBER)?

# https://github.com/0day-ci/linux/commit/5b6f2811172c968d8eb78167825c58557ea91995
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 5b6f2811172c968d8eb78167825c58557ea91995
vim +117 drivers/char/lrng/lrng_selftest.c

5b6f2811172c96 Stephan Müller 2020-01-09   89  static unsigned int lrng_pool_lfsr_selftest(void)
5b6f2811172c96 Stephan Müller 2020-01-09   90  {
5b6f2811172c96 Stephan Müller 2020-01-09   91  	/*
5b6f2811172c96 Stephan Müller 2020-01-09   92  	 * First, 67th and last entry of entropy pool.
5b6f2811172c96 Stephan Müller 2020-01-09   93  	 *
5b6f2811172c96 Stephan Müller 2020-01-09   94  	 * The 67th entry is picked because this one is the first to receive
5b6f2811172c96 Stephan Müller 2020-01-09   95  	 * an entry. As we start with 1 to inject into the LFSR, the
5b6f2811172c96 Stephan Müller 2020-01-09   96  	 * 67th entry should be equal to rol(1, 7) >> 3 considering that
5b6f2811172c96 Stephan Müller 2020-01-09   97  	 * all other values of the LFSR are zero and the the twist value of 0
5b6f2811172c96 Stephan Müller 2020-01-09   98  	 * is applied.
5b6f2811172c96 Stephan Müller 2020-01-09   99  	 */
5b6f2811172c96 Stephan Müller 2020-01-09  100  	static u32 const lrng_lfsr_selftest_result[][3] = {
5b6f2811172c96 Stephan Müller 2020-01-09  101  		{ 0xf56df24a, 0x00000010, 0x0e014939 },
5b6f2811172c96 Stephan Müller 2020-01-09  102  		{ 0x4b130726, 0x00000010, 0x2802f509 },
5b6f2811172c96 Stephan Müller 2020-01-09  103  		{ 0x87279152, 0x00000010, 0x00150000 },
5b6f2811172c96 Stephan Müller 2020-01-09  104  		{ 0x0b67f997, 0x00000010, 0x00150000 },
5b6f2811172c96 Stephan Müller 2020-01-09  105  		{ 0x4fea174f, 0x00000010, 0xcbf4a6ae },
5b6f2811172c96 Stephan Müller 2020-01-09  106  		{ 0x77149108, 0x00000010, 0x77bfadf2 },
5b6f2811172c96 Stephan Müller 2020-01-09  107  		{ 0x1e96037e, 0x00000010, 0x18017e79 },
5b6f2811172c96 Stephan Müller 2020-01-09  108  		{ 0xc84acef2, 0x00000010, 0x6345f7a8 },
5b6f2811172c96 Stephan Müller 2020-01-09  109  		{ 0x6a2eb6df, 0x00000010, 0x03950000 },
5b6f2811172c96 Stephan Müller 2020-01-09  110  	};
5b6f2811172c96 Stephan Müller 2020-01-09  111  	struct lrng_pool *lrng_pool, *lrng_pool_aligned;
5b6f2811172c96 Stephan Müller 2020-01-09  112  	u32 i, ret = LRNG_SELFTEST_PASSED;
5b6f2811172c96 Stephan Müller 2020-01-09  113  
5b6f2811172c96 Stephan Müller 2020-01-09  114  	BUILD_BUG_ON(ARRAY_SIZE(lrng_lfsr_selftest_result) <
5b6f2811172c96 Stephan Müller 2020-01-09  115  							CONFIG_LRNG_POOL_SIZE);
5b6f2811172c96 Stephan Müller 2020-01-09  116  
5b6f2811172c96 Stephan Müller 2020-01-09 @117  	lrng_pool = kzalloc(sizeof(struct lrng_pool) + sizeof(LRNG_KCAPI_ALIGN),
                                                                                               ^^^^^^^^^^^^^^^^^^^^^^^^
Probably just LRNG_KCAPI_ALIGN is intended with no sizeof().

5b6f2811172c96 Stephan Müller 2020-01-09  118  			    GFP_KERNEL);
5b6f2811172c96 Stephan Müller 2020-01-09  119  	if (!lrng_pool)
5b6f2811172c96 Stephan Müller 2020-01-09  120  		return LRNG_SEFLTEST_ERROR_LFSR;
5b6f2811172c96 Stephan Müller 2020-01-09  121  	lrng_pool_aligned = PTR_ALIGN(lrng_pool, sizeof(u32));
5b6f2811172c96 Stephan Müller 2020-01-09  122  
5b6f2811172c96 Stephan Müller 2020-01-09  123  	for (i = 1; i <= LRNG_POOL_SIZE; i++)
5b6f2811172c96 Stephan Müller 2020-01-09  124  		_lrng_pool_lfsr_u32(lrng_pool_aligned, i);
5b6f2811172c96 Stephan Müller 2020-01-09  125  
5b6f2811172c96 Stephan Müller 2020-01-09  126  	if ((atomic_read_u32(&lrng_pool_aligned->pool[0]) !=
5b6f2811172c96 Stephan Müller 2020-01-09  127  	     lrng_lfsr_selftest_result[CONFIG_LRNG_POOL_SIZE][0]) ||
5b6f2811172c96 Stephan Müller 2020-01-09  128  	    (atomic_read_u32(&lrng_pool_aligned->pool[67 &
5b6f2811172c96 Stephan Müller 2020-01-09  129  						      (LRNG_POOL_SIZE - 1)]) !=
5b6f2811172c96 Stephan Müller 2020-01-09  130  	     lrng_lfsr_selftest_result[CONFIG_LRNG_POOL_SIZE][1]) ||
5b6f2811172c96 Stephan Müller 2020-01-09  131  	    (atomic_read_u32(&lrng_pool_aligned->pool[LRNG_POOL_SIZE - 1]) !=
5b6f2811172c96 Stephan Müller 2020-01-09  132  	     lrng_lfsr_selftest_result[CONFIG_LRNG_POOL_SIZE][2])) {
5b6f2811172c96 Stephan Müller 2020-01-09  133  		pr_err("LRNG LFSR self-test FAILED\n");
5b6f2811172c96 Stephan Müller 2020-01-09  134  		ret = LRNG_SEFLTEST_ERROR_LFSR;
5b6f2811172c96 Stephan Müller 2020-01-09  135  	}
5b6f2811172c96 Stephan Müller 2020-01-09  136  
5b6f2811172c96 Stephan Müller 2020-01-09  137  	kfree(lrng_pool);
5b6f2811172c96 Stephan Müller 2020-01-09  138  	return ret;
5b6f2811172c96 Stephan Müller 2020-01-09  139  }
5b6f2811172c96 Stephan Müller 2020-01-09  140  
5b6f2811172c96 Stephan Müller 2020-01-09  141  /*
5b6f2811172c96 Stephan Müller 2020-01-09  142   * The test vectors are generated with the hash_df_testvector_generation tool
5b6f2811172c96 Stephan Müller 2020-01-09  143   * provided as part of the test tool set of the LRNG.
5b6f2811172c96 Stephan Müller 2020-01-09  144   */
5b6f2811172c96 Stephan Müller 2020-01-09  145  static unsigned int lrng_hash_df_selftest(void)
5b6f2811172c96 Stephan Müller 2020-01-09  146  {
5b6f2811172c96 Stephan Müller 2020-01-09  147  	const struct lrng_crypto_cb *crypto_cb = &lrng_cc20_crypto_cb;
5b6f2811172c96 Stephan Müller 2020-01-09  148  
5b6f2811172c96 Stephan Müller 2020-01-09  149  	/*
5b6f2811172c96 Stephan Müller 2020-01-09  150  	 * The size of 45 bytes is chosen arbitrarily. Yet, this size should
5b6f2811172c96 Stephan Müller 2020-01-09  151  	 * ensure that we have at least two hash blocks plus some fraction
5b6f2811172c96 Stephan Müller 2020-01-09  152  	 * of a hash block generated.
5b6f2811172c96 Stephan Müller 2020-01-09  153  	 */
5b6f2811172c96 Stephan Müller 2020-01-09  154  	static u8 const lrng_hash_df_selftest_result[][45] = {
5b6f2811172c96 Stephan Müller 2020-01-09  155  		{
5b6f2811172c96 Stephan Müller 2020-01-09  156  			0x3b, 0xbe, 0x7a, 0xbd, 0x2b, 0x16, 0x02, 0x4c,
5b6f2811172c96 Stephan Müller 2020-01-09  157  			0xfc, 0xd3, 0x02, 0x15, 0xf0, 0x86, 0xd4, 0xdb,
5b6f2811172c96 Stephan Müller 2020-01-09  158  			0x49, 0xec, 0x26, 0x53, 0xd6, 0xc9, 0x6d, 0xad,
5b6f2811172c96 Stephan Müller 2020-01-09  159  			0x24, 0xca, 0x72, 0x89, 0x2c, 0xfa, 0x48, 0x18,
5b6f2811172c96 Stephan Müller 2020-01-09  160  			0xf7, 0x47, 0xb5, 0x2f, 0x92, 0xa2, 0x1b, 0xd9,
5b6f2811172c96 Stephan Müller 2020-01-09  161  			0x24, 0xa7, 0x2f, 0xa2, 0x0b,
5b6f2811172c96 Stephan Müller 2020-01-09  162  		}, {
5b6f2811172c96 Stephan Müller 2020-01-09  163  			0xd2, 0xaa, 0xf9, 0x76, 0x26, 0xc6, 0x13, 0xea,
5b6f2811172c96 Stephan Müller 2020-01-09  164  			0xb8, 0xde, 0xe6, 0x88, 0x8f, 0xc4, 0x7a, 0x7d,
5b6f2811172c96 Stephan Müller 2020-01-09  165  			0x9c, 0xb4, 0x1b, 0xd1, 0xd1, 0x8a, 0x40, 0xc9,
5b6f2811172c96 Stephan Müller 2020-01-09  166  			0xaa, 0x45, 0xa6, 0xb6, 0xb5, 0x6f, 0xf6, 0xbc,
5b6f2811172c96 Stephan Müller 2020-01-09  167  			0xbb, 0x77, 0x37, 0xbc, 0x5a, 0x2d, 0xcc, 0x84,
5b6f2811172c96 Stephan Müller 2020-01-09  168  			0x25, 0x68, 0x5e, 0xba, 0x16,
5b6f2811172c96 Stephan Müller 2020-01-09  169  		}, {
5b6f2811172c96 Stephan Müller 2020-01-09  170  			0x58, 0x66, 0x82, 0x88, 0x29, 0x19, 0xa4, 0xbb,
5b6f2811172c96 Stephan Müller 2020-01-09  171  			0x33, 0x42, 0xc9, 0x72, 0x0d, 0x68, 0x6e, 0xb9,
5b6f2811172c96 Stephan Müller 2020-01-09  172  			0xc6, 0xe0, 0x7a, 0xf9, 0x20, 0xca, 0x6d, 0x18,
5b6f2811172c96 Stephan Müller 2020-01-09  173  			0x35, 0xec, 0xfa, 0x9e, 0xf6, 0x3a, 0xa7, 0xb6,
5b6f2811172c96 Stephan Müller 2020-01-09  174  			0x92, 0x7a, 0xe5, 0xcd, 0xc5, 0x13, 0x9f, 0x65,
5b6f2811172c96 Stephan Müller 2020-01-09  175  			0x6a, 0xe1, 0xe4, 0x3f, 0xb9,
5b6f2811172c96 Stephan Müller 2020-01-09  176  		}, {
5b6f2811172c96 Stephan Müller 2020-01-09  177  			0xdd, 0xf1, 0x34, 0xca, 0x08, 0xe3, 0xce, 0x8a,
5b6f2811172c96 Stephan Müller 2020-01-09  178  			0x26, 0x6b, 0xce, 0x99, 0x8a, 0x84, 0xd2, 0x21,
5b6f2811172c96 Stephan Müller 2020-01-09  179  			0x98, 0x10, 0x95, 0x5f, 0x9f, 0xc3, 0xf2, 0xe4,
5b6f2811172c96 Stephan Müller 2020-01-09  180  			0x79, 0x75, 0xb5, 0x15, 0xa7, 0xa2, 0xf1, 0xc4,
5b6f2811172c96 Stephan Müller 2020-01-09  181  			0xdc, 0x67, 0xcb, 0x67, 0x8c, 0xb2, 0x1b, 0xd5,
5b6f2811172c96 Stephan Müller 2020-01-09  182  			0xd6, 0x8b, 0xc2, 0x34, 0xd6,
5b6f2811172c96 Stephan Müller 2020-01-09  183  		}, {
5b6f2811172c96 Stephan Müller 2020-01-09  184  			0xc3, 0x16, 0x9d, 0xf0, 0x78, 0x15, 0xab, 0xf2,
5b6f2811172c96 Stephan Müller 2020-01-09  185  			0x2f, 0xc9, 0x2e, 0xe1, 0xc6, 0x5e, 0xfa, 0x03,
5b6f2811172c96 Stephan Müller 2020-01-09  186  			0xaf, 0xd4, 0xd5, 0x47, 0x2a, 0xe8, 0x06, 0xe8,
5b6f2811172c96 Stephan Müller 2020-01-09  187  			0x7e, 0x0a, 0x71, 0xc7, 0x0d, 0x39, 0xb1, 0xa9,
5b6f2811172c96 Stephan Müller 2020-01-09  188  			0x5a, 0x49, 0xee, 0x8b, 0x2f, 0xcd, 0xea, 0x96,
5b6f2811172c96 Stephan Müller 2020-01-09  189  			0xcc, 0x08, 0x71, 0xef, 0x9c,
5b6f2811172c96 Stephan Müller 2020-01-09  190  		}, {
5b6f2811172c96 Stephan Müller 2020-01-09  191  			0x1a, 0x3d, 0x70, 0x39, 0xc2, 0x02, 0x4d, 0x3a,
5b6f2811172c96 Stephan Müller 2020-01-09  192  			0xaa, 0x14, 0x20, 0x88, 0x96, 0x4c, 0x7c, 0xe4,
5b6f2811172c96 Stephan Müller 2020-01-09  193  			0xaa, 0x49, 0x89, 0x30, 0x50, 0x96, 0xb6, 0xa7,
5b6f2811172c96 Stephan Müller 2020-01-09  194  			0x55, 0x0a, 0xf8, 0xd2, 0x4e, 0x83, 0x9d, 0x1f,
5b6f2811172c96 Stephan Müller 2020-01-09  195  			0x56, 0x49, 0x13, 0xc6, 0x46, 0x55, 0x73, 0x0d,
5b6f2811172c96 Stephan Müller 2020-01-09  196  			0x74, 0xcd, 0x81, 0xe0, 0x65,
5b6f2811172c96 Stephan Müller 2020-01-09  197  		}, {
5b6f2811172c96 Stephan Müller 2020-01-09  198  			0x4b, 0xf6, 0x49, 0x89, 0x2a, 0x9f, 0x67, 0xd7,
5b6f2811172c96 Stephan Müller 2020-01-09  199  			0xb8, 0x1d, 0xbb, 0x5d, 0xf0, 0x1b, 0x60, 0xb6,
5b6f2811172c96 Stephan Müller 2020-01-09  200  			0xb7, 0xf3, 0x86, 0x6d, 0xe0, 0x04, 0xa1, 0xbc,
5b6f2811172c96 Stephan Müller 2020-01-09  201  			0x3b, 0xb0, 0x10, 0x91, 0xe8, 0x22, 0x67, 0x5b,
5b6f2811172c96 Stephan Müller 2020-01-09  202  			0xe8, 0xf0, 0x4f, 0x82, 0x70, 0xc7, 0xe1, 0xc8,
5b6f2811172c96 Stephan Müller 2020-01-09  203  			0xd8, 0xad, 0x70, 0xcf, 0xf6,
5b6f2811172c96 Stephan Müller 2020-01-09  204  		}, {
5b6f2811172c96 Stephan Müller 2020-01-09  205  			0x60, 0x1f, 0x71, 0x07, 0x92, 0xae, 0xa0, 0x24,
5b6f2811172c96 Stephan Müller 2020-01-09  206  			0xb6, 0xa4, 0x10, 0x70, 0x1f, 0x94, 0x51, 0x9a,
5b6f2811172c96 Stephan Müller 2020-01-09  207  			0x5a, 0x81, 0xc4, 0x46, 0x78, 0x56, 0x71, 0xdd,
5b6f2811172c96 Stephan Müller 2020-01-09  208  			0x45, 0x63, 0x01, 0x34, 0x87, 0x79, 0xb4, 0xd5,
5b6f2811172c96 Stephan Müller 2020-01-09  209  			0x91, 0x79, 0xb9, 0x93, 0x11, 0x44, 0x50, 0xad,
5b6f2811172c96 Stephan Müller 2020-01-09  210  			0x64, 0x7e, 0x5c, 0xec, 0x16,
5b6f2811172c96 Stephan Müller 2020-01-09  211  		}, {
5b6f2811172c96 Stephan Müller 2020-01-09  212  			0x49, 0x2f, 0xa0, 0x45, 0xf8, 0xb0, 0x80, 0x88,
5b6f2811172c96 Stephan Müller 2020-01-09  213  			0x79, 0xeb, 0xb6, 0x82, 0x1c, 0xf3, 0x67, 0xc4,
5b6f2811172c96 Stephan Müller 2020-01-09  214  			0x88, 0x88, 0xe9, 0x75, 0x20, 0x54, 0x78, 0xc6,
5b6f2811172c96 Stephan Müller 2020-01-09  215  			0x5c, 0x59, 0xcf, 0xd9, 0x73, 0x12, 0x17, 0xf4,
5b6f2811172c96 Stephan Müller 2020-01-09  216  			0x30, 0x9c, 0xb7, 0x21, 0x45, 0xe2, 0xb6, 0x0c,
5b6f2811172c96 Stephan Müller 2020-01-09  217  			0x0c, 0xeb, 0x1b, 0xdc, 0xdc,
5b6f2811172c96 Stephan Müller 2020-01-09  218  		}
5b6f2811172c96 Stephan Müller 2020-01-09  219  	};
5b6f2811172c96 Stephan Müller 2020-01-09  220  	struct lrng_pool *lrng_pool, *lrng_pool_aligned;
5b6f2811172c96 Stephan Müller 2020-01-09  221  	u8 hash_df[sizeof(lrng_hash_df_selftest_result[0])]
5b6f2811172c96 Stephan Müller 2020-01-09  222  							__aligned(sizeof(u32));
5b6f2811172c96 Stephan Müller 2020-01-09  223  	u32 generated;
5b6f2811172c96 Stephan Müller 2020-01-09  224  	int ret = 0;
5b6f2811172c96 Stephan Müller 2020-01-09  225  
5b6f2811172c96 Stephan Müller 2020-01-09  226  	BUILD_BUG_ON(ARRAY_SIZE(lrng_hash_df_selftest_result) <
5b6f2811172c96 Stephan Müller 2020-01-09  227  							CONFIG_LRNG_POOL_SIZE);
5b6f2811172c96 Stephan Müller 2020-01-09  228  
5b6f2811172c96 Stephan Müller 2020-01-09 @229  	lrng_pool = kzalloc(sizeof(struct lrng_pool) + sizeof(LRNG_KCAPI_ALIGN),
                                                                                               ^^^^^^^^^^^^^^^^^^^^^^^^
Here too.


5b6f2811172c96 Stephan Müller 2020-01-09  230  			    GFP_KERNEL);
5b6f2811172c96 Stephan Müller 2020-01-09  231  	if (!lrng_pool)
5b6f2811172c96 Stephan Müller 2020-01-09  232  		return LRNG_SEFLTEST_ERROR_HASHDF;
5b6f2811172c96 Stephan Müller 2020-01-09  233  	lrng_pool_aligned = PTR_ALIGN(lrng_pool, sizeof(u32));
5b6f2811172c96 Stephan Müller 2020-01-09  234  
5b6f2811172c96 Stephan Müller 2020-01-09  235  	generated = __lrng_pool_hash_df(crypto_cb, NULL, lrng_pool_aligned,
5b6f2811172c96 Stephan Müller 2020-01-09  236  					hash_df, sizeof(hash_df) << 3);
5b6f2811172c96 Stephan Müller 2020-01-09  237  
5b6f2811172c96 Stephan Müller 2020-01-09  238  	if ((generated >> 3) != sizeof(hash_df) ||
5b6f2811172c96 Stephan Müller 2020-01-09  239  	    memcmp(hash_df, lrng_hash_df_selftest_result[CONFIG_LRNG_POOL_SIZE],
5b6f2811172c96 Stephan Müller 2020-01-09  240  		   sizeof(hash_df))) {
5b6f2811172c96 Stephan Müller 2020-01-09  241  		pr_err("LRNG Hash DF self-test FAILED\n");
5b6f2811172c96 Stephan Müller 2020-01-09  242  		ret = LRNG_SEFLTEST_ERROR_HASHDF;
5b6f2811172c96 Stephan Müller 2020-01-09  243  	}
5b6f2811172c96 Stephan Müller 2020-01-09  244  
5b6f2811172c96 Stephan Müller 2020-01-09  245  	kfree(lrng_pool);
5b6f2811172c96 Stephan Müller 2020-01-09  246  	return ret;
5b6f2811172c96 Stephan Müller 2020-01-09  247  }

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation

^ permalink raw reply

* Re: [PATCH 2/4] mm: introduce external memory hinting API
From: Christian Brauner @ 2020-01-13 10:42 UTC (permalink / raw)
  To: Kirill Tkhai, Michal Hocko, Minchan Kim
  Cc: Andrew Morton, LKML, linux-mm, linux-api-u79uwXL29TY76Z2rM5mHXA,
	oleksandr-H+wXaHxf7aLQT0dZR+AlfA, Suren Baghdasaryan, Tim Murray,
	Daniel Colascione, Sandeep Patil, Sonny Rao, Brian Geffon,
	Johannes Weiner, Shakeel Butt, John Dias
In-Reply-To: <56ea0927-ad2e-3fbd-3366-3813330f6cec-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org>

On Mon, Jan 13, 2020 at 11:47:11AM +0300, Kirill Tkhai wrote:
> On 11.01.2020 00:34, Minchan Kim wrote:
> > There are usecases that System Management Software(SMS) want to give
> > a memory hint to other processes because it's not known to the
> > application. In the case of Android, ActivityManagerService daemon
> > manges app's life cycle and that daemon must be able to initiate
> > reclaim on its own without any app involvement.
> > 
> > To solve the issue, this patch introduces new syscall process_madvise(2).
> > It uses pidfd of an external processs to give the hint.
> > 
> >  int process_madvise(int pidfd, void *addr, size_t length, int advise,
> > 			unsigned long flag);
> > 
> > Since it could affect other process's address range, only privileged
> > process(CAP_SYS_PTRACE) or something else(e.g., being the same UID)
> > gives it the right to ptrace the process could use it successfully.
> > The flag argument is reserved for future use if we need to extend the
> > API.
> > 
> > Supporting all hints madvise has/will supported/support to process_madvise
> > is rather risky. Because we are not sure all hints make sense from external
> > process and implementation for the hint may rely on the caller being
> > in the current context so it could be error-prone. Thus, I just limited
> > hints as MADV_[COLD|PAGEOUT] in this patch.
> > 
> > If someone want to add other hints, we could hear hear the usecase and
> > review it for each hint. It's more safe for maintainace rather than
> > introducing a buggy syscall but hard to fix it later.
> > 
> > Signed-off-by: Minchan Kim <minchan-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
> >  arch/alpha/kernel/syscalls/syscall.tbl      |  1 +
> >  arch/arm/tools/syscall.tbl                  |  1 +
> >  arch/arm64/include/asm/unistd.h             |  2 +-
> >  arch/arm64/include/asm/unistd32.h           |  2 +
> >  arch/ia64/kernel/syscalls/syscall.tbl       |  1 +
> >  arch/m68k/kernel/syscalls/syscall.tbl       |  1 +
> >  arch/microblaze/kernel/syscalls/syscall.tbl |  1 +
> >  arch/mips/kernel/syscalls/syscall_n32.tbl   |  1 +
> >  arch/mips/kernel/syscalls/syscall_n64.tbl   |  1 +
> >  arch/parisc/kernel/syscalls/syscall.tbl     |  1 +
> >  arch/powerpc/kernel/syscalls/syscall.tbl    |  1 +
> >  arch/s390/kernel/syscalls/syscall.tbl       |  1 +
> >  arch/sh/kernel/syscalls/syscall.tbl         |  1 +
> >  arch/sparc/kernel/syscalls/syscall.tbl      |  1 +
> >  arch/x86/entry/syscalls/syscall_32.tbl      |  1 +
> >  arch/x86/entry/syscalls/syscall_64.tbl      |  1 +
> >  arch/xtensa/kernel/syscalls/syscall.tbl     |  1 +
> >  include/linux/syscalls.h                    |  2 +
> >  include/uapi/asm-generic/unistd.h           |  5 +-
> >  kernel/sys_ni.c                             |  1 +
> >  mm/madvise.c                                | 64 +++++++++++++++++++++
> >  21 files changed, 89 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
> > index e56950f23b49..776c61803315 100644
> > --- a/arch/alpha/kernel/syscalls/syscall.tbl
> > +++ b/arch/alpha/kernel/syscalls/syscall.tbl
> > @@ -477,3 +477,4 @@
> >  # 545 reserved for clone3
> >  546	common	watch_devices			sys_watch_devices
> >  547	common	openat2				sys_openat2
> > +548	common	process_madvise			sys_process_madvise
> > diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
> > index 7fb2f4d59210..a43381542276 100644
> > --- a/arch/arm/tools/syscall.tbl
> > +++ b/arch/arm/tools/syscall.tbl
> > @@ -451,3 +451,4 @@
> >  435	common	clone3				sys_clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +438	common	process_madvise			sys_process_madvise
> > diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
> > index 8aa00ccb0b96..b722e47377a5 100644
> > --- a/arch/arm64/include/asm/unistd.h
> > +++ b/arch/arm64/include/asm/unistd.h
> > @@ -38,7 +38,7 @@
> >  #define __ARM_NR_compat_set_tls		(__ARM_NR_COMPAT_BASE + 5)
> >  #define __ARM_NR_COMPAT_END		(__ARM_NR_COMPAT_BASE + 0x800)
> >  
> > -#define __NR_compat_syscalls		438
> > +#define __NR_compat_syscalls		439
> >  #endif
> >  
> >  #define __ARCH_WANT_SYS_CLONE
> > diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
> > index 31f0ce25719e..5c82557d408f 100644
> > --- a/arch/arm64/include/asm/unistd32.h
> > +++ b/arch/arm64/include/asm/unistd32.h
> > @@ -883,6 +883,8 @@ __SYSCALL(__NR_clone3, sys_clone3)
> >  __SYSCALL(__NR_watch_devices, sys_watch_devices)
> >  #define __NR_openat2 437
> >  __SYSCALL(__NR_openat2, sys_openat2)
> > +#define __NR_openat2 438
> > +__SYSCALL(__NR_process_madvise, process_madvise)
> >  
> >  /*
> >   * Please add new compat syscalls above this comment and update
> > diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl
> > index b9aa59931905..c156abc9a298 100644
> > --- a/arch/ia64/kernel/syscalls/syscall.tbl
> > +++ b/arch/ia64/kernel/syscalls/syscall.tbl
> > @@ -358,3 +358,4 @@
> >  # 435 reserved for clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +438	common	process_madvise			sys_process_madvise
> > diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
> > index 868c1ef89d35..5b6034b6650f 100644
> > --- a/arch/m68k/kernel/syscalls/syscall.tbl
> > +++ b/arch/m68k/kernel/syscalls/syscall.tbl
> > @@ -437,3 +437,4 @@
> >  # 435 reserved for clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +438	common	process_madvise			sys_process_madvise
> > diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
> > index 544b4cef18b3..4bef584af09c 100644
> > --- a/arch/microblaze/kernel/syscalls/syscall.tbl
> > +++ b/arch/microblaze/kernel/syscalls/syscall.tbl
> > @@ -443,3 +443,4 @@
> >  435	common	clone3				sys_clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +438	common	process_madvise			sys_process_madvise
> > diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
> > index 05e8aee5dae7..94fbd0fcccce 100644
> > --- a/arch/mips/kernel/syscalls/syscall_n32.tbl
> > +++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
> > @@ -376,3 +376,4 @@
> >  435	n32	clone3				__sys_clone3
> >  436	n32	watch_devices			sys_watch_devices
> >  437	n32	openat2				sys_openat2
> > +437	n32	process_madivse			sys_process_madvise
> 
> 438. And several places below has the same mistake.
> 
> > diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
> > index 24d6c01328fb..4e6982c429d5 100644
> > --- a/arch/mips/kernel/syscalls/syscall_n64.tbl
> > +++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
> > @@ -352,3 +352,4 @@
> >  435	n64	clone3				__sys_clone3
> >  436	n64	watch_devices			sys_watch_devices
> >  437	n64	openat2				sys_openat2
> > +437	n64	process_madvise			sys_process_madvise
> > diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
> > index 4b5f77a4e1a2..3aa990caf9dc 100644
> > --- a/arch/parisc/kernel/syscalls/syscall.tbl
> > +++ b/arch/parisc/kernel/syscalls/syscall.tbl
> > @@ -435,3 +435,4 @@
> >  435	common	clone3				sys_clone3_wrapper
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +437	common	process_madvise			sys_process_madvise
> > diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
> > index 9716dc85a517..30e727a23f33 100644
> > --- a/arch/powerpc/kernel/syscalls/syscall.tbl
> > +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
> > @@ -519,3 +519,4 @@
> >  435	nospu	clone3				ppc_clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +437	common	process_madvise			sys_process_madvise
> > diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
> > index 7da330f8b03e..75722e5ff496 100644
> > --- a/arch/s390/kernel/syscalls/syscall.tbl
> > +++ b/arch/s390/kernel/syscalls/syscall.tbl
> > @@ -440,3 +440,4 @@
> >  435  common	clone3			sys_clone3			sys_clone3
> >  436  common	watch_devices		sys_watch_devices		sys_watch_devices
> >  437  common	openat2			sys_openat2			sys_openat2
> > +437  common	process_madvise		sys_process_madvise		sys_process_madvise
> > diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
> > index bb7e68e25337..7d7bc7befad3 100644
> > --- a/arch/sh/kernel/syscalls/syscall.tbl
> > +++ b/arch/sh/kernel/syscalls/syscall.tbl
> > @@ -440,3 +440,4 @@
> >  # 435 reserved for clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +437	common	process_madvise			sys_process_madvise
> > diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
> > index 646a1fad7218..581d331ff62f 100644
> > --- a/arch/sparc/kernel/syscalls/syscall.tbl
> > +++ b/arch/sparc/kernel/syscalls/syscall.tbl
> > @@ -483,3 +483,4 @@
> >  # 435 reserved for clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2			sys_openat2
> > +437	common	process_madvise		sys_process_madvise
> > diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> > index 57c53acee290..76a2c266fe7e 100644
> > --- a/arch/x86/entry/syscalls/syscall_32.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> > @@ -442,3 +442,4 @@
> >  435	i386	clone3			sys_clone3			__ia32_sys_clone3
> >  436	i386	watch_devices		sys_watch_devices		__ia32_sys_watch_devices
> >  437	i386	openat2			sys_openat2			__ia32_sys_openat2
> > +438	i386	process_madvise		sys_process_madvise		__ia32_sys_process_madvise
> > diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> > index 1dd8d21f6500..b697cd8620cb 100644
> > --- a/arch/x86/entry/syscalls/syscall_64.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> > @@ -359,6 +359,7 @@
> >  435	common	clone3			__x64_sys_clone3/ptregs
> >  436	common	watch_devices		__x64_sys_watch_devices
> >  437	common	openat2			__x64_sys_openat2
> > +438	common	process_madvise		__x64_sys_process_madvise
> >  
> >  #
> >  # x32-specific system call numbers start at 512 to avoid cache impact
> > diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
> > index 0f48ab7bd75b..2e9813ecfd7d 100644
> > --- a/arch/xtensa/kernel/syscalls/syscall.tbl
> > +++ b/arch/xtensa/kernel/syscalls/syscall.tbl
> > @@ -408,3 +408,4 @@
> >  435	common	clone3				sys_clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +438	common	process_madvise			sys_process_madvise
> > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> > index 433c8c85636e..1b58a11ff49f 100644
> > --- a/include/linux/syscalls.h
> > +++ b/include/linux/syscalls.h
> > @@ -877,6 +877,8 @@ asmlinkage long sys_munlockall(void);
> >  asmlinkage long sys_mincore(unsigned long start, size_t len,
> >  				unsigned char __user * vec);
> >  asmlinkage long sys_madvise(unsigned long start, size_t len, int behavior);
> > +asmlinkage long sys_process_madvise(int pidfd, unsigned long start,
> > +			size_t len, int behavior, unsigned long flags);
> >  asmlinkage long sys_remap_file_pages(unsigned long start, unsigned long size,
> >  			unsigned long prot, unsigned long pgoff,
> >  			unsigned long flags);
> > diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> > index 33f3856a9c3c..4bcd8d366f38 100644
> > --- a/include/uapi/asm-generic/unistd.h
> > +++ b/include/uapi/asm-generic/unistd.h
> > @@ -856,8 +856,11 @@ __SYSCALL(__NR_watch_devices, sys_watch_devices)
> >  #define __NR_openat2 437
> >  __SYSCALL(__NR_openat2, sys_openat2)
> >  
> > +#define __NR_openat2 438
> > +__SYSCALL(__NR_process_madvise, sys_process_madvise)
> > +
> >  #undef __NR_syscalls
> > -#define __NR_syscalls 438
> > +#define __NR_syscalls 439
> >  
> >  /*
> >   * 32 bit systems traditionally used different
> > diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
> > index 0e9b275260f8..10ce5eac8b4b 100644
> > --- a/kernel/sys_ni.c
> > +++ b/kernel/sys_ni.c
> > @@ -281,6 +281,7 @@ COND_SYSCALL(mlockall);
> >  COND_SYSCALL(munlockall);
> >  COND_SYSCALL(mincore);
> >  COND_SYSCALL(madvise);
> > +COND_SYSCALL(process_madvise);
> >  COND_SYSCALL(remap_file_pages);
> >  COND_SYSCALL(mbind);
> >  COND_SYSCALL_COMPAT(mbind);
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 0c901de531e4..e15dfb4df7bf 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/falloc.h>
> >  #include <linux/fadvise.h>
> >  #include <linux/sched.h>
> > +#include <linux/sched/mm.h>
> >  #include <linux/ksm.h>
> >  #include <linux/fs.h>
> >  #include <linux/file.h>
> > @@ -993,6 +994,18 @@ madvise_behavior_valid(int behavior)
> >  	}
> >  }
> >  
> > +static bool
> > +process_madvise_behavior_valid(int behavior)
> > +{
> > +	switch (behavior) {
> > +	case MADV_COLD:
> > +	case MADV_PAGEOUT:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> >  /*
> >   * madvise_common - request behavior hint to address range of the target process
> >   *
> > @@ -1169,3 +1182,54 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> >  {
> >  	return madvise_common(current, current->mm, start, len_in, behavior);
> >  }
> > +
> > +SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
> > +		size_t, len_in, int, behavior, unsigned long, flags)
> 
> I don't like the interface. The fact we have pidfd does not mean,
> we have to use it for new syscalls always. A user may want to set
> madvise for specific pid from console and pass pid as argument.
> pidfd would be an overkill in this case.
> We usually call "kill -9 pid" from console. Why shouldn't process_madvise()
> allow this?
> 
> I suggent to extend first argument to work with both pid and pidfd.
> Look at what we have for waitid(idtype, id_t id, ...) for example:
> 
>        idtype == P_PID
>               Wait for the child whose process ID matches id.
> 
>        idtype == P_PIDFD (since Linux 5.4)
>               Wait for the child referred to by the PID file descriptor specified in id.  (See pidfd_open(2) for  further  information  on
>               PID file descriptors.)
> 
> We may use @flags argument for this.

Sorry for chiming in just a comment. Overall, I don't particularly care
how or if you integrate pidfd here. One thing I would like to point out
is that we're working on a patch to place new features under pidfd
specific flags. This e.g. means a pidfd would be only be able to be used
for madvise operations (or getfd operations) if it was created with that
specific flag set making it easier to share them with other processes.
So if you integrate them here I would be quite thankful if you target
the patchset for the v5.7 merge window, not for v5.6.

Thanks!
Christian

^ permalink raw reply

* Re: [PATCH v27 12/12] LRNG - add power-on and runtime self-tests
From: Stephan Mueller @ 2020-01-13 10:46 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: kbuild-hn68Rpc1hR1g9hUCZPvPmw, kbuild-all-hn68Rpc1hR1g9hUCZPvPmw,
	Arnd Bergmann, Greg Kroah-Hartman,
	linux-crypto-u79uwXL29TY76Z2rM5mHXA, LKML,
	linux-api-u79uwXL29TY76Z2rM5mHXA, Eric W. Biederman,
	Alexander E. Patrakov, Ahmed S. Darwish, Theodore Y. Ts'o,
	Willy Tarreau, Matthew Garrett, Vito Caputo, Andreas Dilger,
	Jan Kara, Ray Strode, William Jon McCann, zhangjs,
	Andy Lutomirski, Florian Weimer, Lennart Poettering <mzxr>
In-Reply-To: <20200113103941.GC9488@kadam>

Am Montag, 13. Januar 2020, 11:39:41 CET schrieb Dan Carpenter:

Hi Dan,

> Hi "Stephan,
> 
> url:   
> https://github.com/0day-ci/linux/commits/Stephan-M-ller/dev-random-a-new-ap
> proach-with-full-SP800-90B/20200110-084934 base:  
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git
> 68faa679b8be1a74e6663c21c3a9d25d32f1c079
> 
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Reported-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> 
> smatch warnings:
> drivers/char/lrng/lrng_selftest.c:117 lrng_pool_lfsr_selftest() warn:
> sizeof(NUMBER)? drivers/char/lrng/lrng_selftest.c:229
> lrng_hash_df_selftest() warn: sizeof(NUMBER)?
> 
> #
> https://github.com/0day-ci/linux/commit/5b6f2811172c968d8eb78167825c58557ea
> 91995 git remote add linux-review https://github.com/0day-ci/linux
> git remote update linux-review
> git checkout 5b6f2811172c968d8eb78167825c58557ea91995
> vim +117 drivers/char/lrng/lrng_selftest.c
> 
> 5b6f2811172c96 Stephan Müller 2020-01-09   89  static unsigned int
> lrng_pool_lfsr_selftest(void) 5b6f2811172c96 Stephan Müller 2020-01-09   90
>  {
> 5b6f2811172c96 Stephan Müller 2020-01-09   91  	/*
> 5b6f2811172c96 Stephan Müller 2020-01-09   92  	 * First, 67th and last
> entry of entropy pool. 5b6f2811172c96 Stephan Müller 2020-01-09   93  	 *
> 5b6f2811172c96 Stephan Müller 2020-01-09   94  	 * The 67th entry is picked
> because this one is the first to receive 5b6f2811172c96 Stephan Müller
> 2020-01-09   95  	 * an entry. As we start with 1 to inject into the 
LFSR,
> the 5b6f2811172c96 Stephan Müller 2020-01-09   96  	 * 67th entry should 
be
> equal to rol(1, 7) >> 3 considering that 5b6f2811172c96 Stephan Müller
> 2020-01-09   97  	 * all other values of the LFSR are zero and the the
> twist value of 0 5b6f2811172c96 Stephan Müller 2020-01-09   98  	 * is
> applied.
> 5b6f2811172c96 Stephan Müller 2020-01-09   99  	 */
> 5b6f2811172c96 Stephan Müller 2020-01-09  100  	static u32 const
> lrng_lfsr_selftest_result[][3] = { 5b6f2811172c96 Stephan Müller 2020-01-09
>  101  		{ 0xf56df24a, 0x00000010, 0x0e014939 }, 5b6f2811172c96 Stephan
> Müller 2020-01-09  102  		{ 0x4b130726, 0x00000010, 0x2802f509 
},
> 5b6f2811172c96 Stephan Müller 2020-01-09  103  		{ 0x87279152, 
0x00000010,
> 0x00150000 }, 5b6f2811172c96 Stephan Müller 2020-01-09  104  		{
> 0x0b67f997, 0x00000010, 0x00150000 }, 5b6f2811172c96 Stephan Müller
> 2020-01-09  105  		{ 0x4fea174f, 0x00000010, 0xcbf4a6ae }, 
5b6f2811172c96
> Stephan Müller 2020-01-09  106  		{ 0x77149108, 0x00000010, 
0x77bfadf2 },
> 5b6f2811172c96 Stephan Müller 2020-01-09  107  		{ 0x1e96037e, 
0x00000010,
> 0x18017e79 }, 5b6f2811172c96 Stephan Müller 2020-01-09  108  		{
> 0xc84acef2, 0x00000010, 0x6345f7a8 }, 5b6f2811172c96 Stephan Müller
> 2020-01-09  109  		{ 0x6a2eb6df, 0x00000010, 0x03950000 }, 
5b6f2811172c96
> Stephan Müller 2020-01-09  110  	};
> 5b6f2811172c96 Stephan Müller 2020-01-09  111  	struct lrng_pool *lrng_pool,
> *lrng_pool_aligned; 5b6f2811172c96 Stephan Müller 2020-01-09  112  	u32 
i,
> ret = LRNG_SELFTEST_PASSED; 5b6f2811172c96 Stephan Müller 2020-01-09  113
> 5b6f2811172c96 Stephan Müller 2020-01-09  114 
> 	BUILD_BUG_ON(ARRAY_SIZE(lrng_lfsr_selftest_result) < 5b6f2811172c96
> Stephan Müller 2020-01-09  115  						
	CONFIG_LRNG_POOL_SIZE);
> 5b6f2811172c96 Stephan Müller 2020-01-09  116
> 5b6f2811172c96 Stephan Müller 2020-01-09 @117  	lrng_pool =
> kzalloc(sizeof(struct lrng_pool) + sizeof(LRNG_KCAPI_ALIGN),
> ^^^^^^^^^^^^^^^^^^^^^^^^ Probably just LRNG_KCAPI_ALIGN is intended with no
> sizeof().


Yes, absolutely. Fixed in both cases.

Thank you!
> 
> 5b6f2811172c96 Stephan Müller 2020-01-09  118  			    
GFP_KERNEL);
> 5b6f2811172c96 Stephan Müller 2020-01-09  119  	if (!lrng_pool)
> 5b6f2811172c96 Stephan Müller 2020-01-09  120  		return
> LRNG_SEFLTEST_ERROR_LFSR; 5b6f2811172c96 Stephan Müller 2020-01-09  121 
> 	lrng_pool_aligned = PTR_ALIGN(lrng_pool, sizeof(u32)); 5b6f2811172c96
> Stephan Müller 2020-01-09  122
> 5b6f2811172c96 Stephan Müller 2020-01-09  123  	for (i = 1; i <=
> LRNG_POOL_SIZE; i++) 5b6f2811172c96 Stephan Müller 2020-01-09  124 
> 		_lrng_pool_lfsr_u32(lrng_pool_aligned, i); 5b6f2811172c96 
Stephan Müller
> 2020-01-09  125
> 5b6f2811172c96 Stephan Müller 2020-01-09  126  	if
> ((atomic_read_u32(&lrng_pool_aligned->pool[0]) != 5b6f2811172c96 Stephan
> Müller 2020-01-09  127  	    
> lrng_lfsr_selftest_result[CONFIG_LRNG_POOL_SIZE][0]) || 5b6f2811172c96
> Stephan Müller 2020-01-09  128  	   
> (atomic_read_u32(&lrng_pool_aligned->pool[67 & 5b6f2811172c96 Stephan
> Müller 2020-01-09  129  						      
(LRNG_POOL_SIZE - 1)]) !=
> 5b6f2811172c96 Stephan Müller 2020-01-09  130  	    
> lrng_lfsr_selftest_result[CONFIG_LRNG_POOL_SIZE][1]) || 5b6f2811172c96
> Stephan Müller 2020-01-09  131  	   
> (atomic_read_u32(&lrng_pool_aligned->pool[LRNG_POOL_SIZE - 1]) !=
> 5b6f2811172c96 Stephan Müller 2020-01-09  132  	    
> lrng_lfsr_selftest_result[CONFIG_LRNG_POOL_SIZE][2])) { 5b6f2811172c96
> Stephan Müller 2020-01-09  133  		pr_err("LRNG LFSR self-test 
FAILED\n");
> 5b6f2811172c96 Stephan Müller 2020-01-09  134  		ret =
> LRNG_SEFLTEST_ERROR_LFSR; 5b6f2811172c96 Stephan Müller 2020-01-09  135  	
}
> 5b6f2811172c96 Stephan Müller 2020-01-09  136
> 5b6f2811172c96 Stephan Müller 2020-01-09  137  	kfree(lrng_pool);
> 5b6f2811172c96 Stephan Müller 2020-01-09  138  	return ret;
> 5b6f2811172c96 Stephan Müller 2020-01-09  139  }
> 5b6f2811172c96 Stephan Müller 2020-01-09  140
> 5b6f2811172c96 Stephan Müller 2020-01-09  141  /*
> 5b6f2811172c96 Stephan Müller 2020-01-09  142   * The test vectors are
> generated with the hash_df_testvector_generation tool 5b6f2811172c96
> Stephan Müller 2020-01-09  143   * provided as part of the test tool set of
> the LRNG. 5b6f2811172c96 Stephan Müller 2020-01-09  144   */
> 5b6f2811172c96 Stephan Müller 2020-01-09  145  static unsigned int
> lrng_hash_df_selftest(void) 5b6f2811172c96 Stephan Müller 2020-01-09  146 
> {
> 5b6f2811172c96 Stephan Müller 2020-01-09  147  	const struct lrng_crypto_cb
> *crypto_cb = &lrng_cc20_crypto_cb; 5b6f2811172c96 Stephan Müller 2020-01-09
>  148
> 5b6f2811172c96 Stephan Müller 2020-01-09  149  	/*
> 5b6f2811172c96 Stephan Müller 2020-01-09  150  	 * The size of 45 bytes is
> chosen arbitrarily. Yet, this size should 5b6f2811172c96 Stephan Müller
> 2020-01-09  151  	 * ensure that we have at least two hash blocks plus 
some
> fraction 5b6f2811172c96 Stephan Müller 2020-01-09  152  	 * of a hash block
> generated. 5b6f2811172c96 Stephan Müller 2020-01-09  153  	 */
> 5b6f2811172c96 Stephan Müller 2020-01-09  154  	static u8 const
> lrng_hash_df_selftest_result[][45] = { 5b6f2811172c96 Stephan Müller
> 2020-01-09  155  		{
> 5b6f2811172c96 Stephan Müller 2020-01-09  156  			0x3b, 0xbe, 
0x7a, 0xbd,
> 0x2b, 0x16, 0x02, 0x4c, 5b6f2811172c96 Stephan Müller 2020-01-09  157 
> 			0xfc, 0xd3, 0x02, 0x15, 0xf0, 0x86, 0xd4, 0xdb, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  158  			0x49, 0xec, 0x26, 0x53, 0xd6, 
0xc9, 0x6d, 0xad,
> 5b6f2811172c96 Stephan Müller 2020-01-09  159  			0x24, 0xca, 
0x72, 0x89,
> 0x2c, 0xfa, 0x48, 0x18, 5b6f2811172c96 Stephan Müller 2020-01-09  160 
> 			0xf7, 0x47, 0xb5, 0x2f, 0x92, 0xa2, 0x1b, 0xd9, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  161  			0x24, 0xa7, 0x2f, 0xa2, 0x0b, 
5b6f2811172c96
> Stephan Müller 2020-01-09  162  		}, {
> 5b6f2811172c96 Stephan Müller 2020-01-09  163  			0xd2, 0xaa, 
0xf9, 0x76,
> 0x26, 0xc6, 0x13, 0xea, 5b6f2811172c96 Stephan Müller 2020-01-09  164 
> 			0xb8, 0xde, 0xe6, 0x88, 0x8f, 0xc4, 0x7a, 0x7d, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  165  			0x9c, 0xb4, 0x1b, 0xd1, 0xd1, 
0x8a, 0x40, 0xc9,
> 5b6f2811172c96 Stephan Müller 2020-01-09  166  			0xaa, 0x45, 
0xa6, 0xb6,
> 0xb5, 0x6f, 0xf6, 0xbc, 5b6f2811172c96 Stephan Müller 2020-01-09  167 
> 			0xbb, 0x77, 0x37, 0xbc, 0x5a, 0x2d, 0xcc, 0x84, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  168  			0x25, 0x68, 0x5e, 0xba, 0x16, 
5b6f2811172c96
> Stephan Müller 2020-01-09  169  		}, {
> 5b6f2811172c96 Stephan Müller 2020-01-09  170  			0x58, 0x66, 
0x82, 0x88,
> 0x29, 0x19, 0xa4, 0xbb, 5b6f2811172c96 Stephan Müller 2020-01-09  171 
> 			0x33, 0x42, 0xc9, 0x72, 0x0d, 0x68, 0x6e, 0xb9, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  172  			0xc6, 0xe0, 0x7a, 0xf9, 0x20, 
0xca, 0x6d, 0x18,
> 5b6f2811172c96 Stephan Müller 2020-01-09  173  			0x35, 0xec, 
0xfa, 0x9e,
> 0xf6, 0x3a, 0xa7, 0xb6, 5b6f2811172c96 Stephan Müller 2020-01-09  174 
> 			0x92, 0x7a, 0xe5, 0xcd, 0xc5, 0x13, 0x9f, 0x65, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  175  			0x6a, 0xe1, 0xe4, 0x3f, 0xb9, 
5b6f2811172c96
> Stephan Müller 2020-01-09  176  		}, {
> 5b6f2811172c96 Stephan Müller 2020-01-09  177  			0xdd, 0xf1, 
0x34, 0xca,
> 0x08, 0xe3, 0xce, 0x8a, 5b6f2811172c96 Stephan Müller 2020-01-09  178 
> 			0x26, 0x6b, 0xce, 0x99, 0x8a, 0x84, 0xd2, 0x21, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  179  			0x98, 0x10, 0x95, 0x5f, 0x9f, 
0xc3, 0xf2, 0xe4,
> 5b6f2811172c96 Stephan Müller 2020-01-09  180  			0x79, 0x75, 
0xb5, 0x15,
> 0xa7, 0xa2, 0xf1, 0xc4, 5b6f2811172c96 Stephan Müller 2020-01-09  181 
> 			0xdc, 0x67, 0xcb, 0x67, 0x8c, 0xb2, 0x1b, 0xd5, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  182  			0xd6, 0x8b, 0xc2, 0x34, 0xd6, 
5b6f2811172c96
> Stephan Müller 2020-01-09  183  		}, {
> 5b6f2811172c96 Stephan Müller 2020-01-09  184  			0xc3, 0x16, 
0x9d, 0xf0,
> 0x78, 0x15, 0xab, 0xf2, 5b6f2811172c96 Stephan Müller 2020-01-09  185 
> 			0x2f, 0xc9, 0x2e, 0xe1, 0xc6, 0x5e, 0xfa, 0x03, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  186  			0xaf, 0xd4, 0xd5, 0x47, 0x2a, 
0xe8, 0x06, 0xe8,
> 5b6f2811172c96 Stephan Müller 2020-01-09  187  			0x7e, 0x0a, 
0x71, 0xc7,
> 0x0d, 0x39, 0xb1, 0xa9, 5b6f2811172c96 Stephan Müller 2020-01-09  188 
> 			0x5a, 0x49, 0xee, 0x8b, 0x2f, 0xcd, 0xea, 0x96, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  189  			0xcc, 0x08, 0x71, 0xef, 0x9c, 
5b6f2811172c96
> Stephan Müller 2020-01-09  190  		}, {
> 5b6f2811172c96 Stephan Müller 2020-01-09  191  			0x1a, 0x3d, 
0x70, 0x39,
> 0xc2, 0x02, 0x4d, 0x3a, 5b6f2811172c96 Stephan Müller 2020-01-09  192 
> 			0xaa, 0x14, 0x20, 0x88, 0x96, 0x4c, 0x7c, 0xe4, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  193  			0xaa, 0x49, 0x89, 0x30, 0x50, 
0x96, 0xb6, 0xa7,
> 5b6f2811172c96 Stephan Müller 2020-01-09  194  			0x55, 0x0a, 
0xf8, 0xd2,
> 0x4e, 0x83, 0x9d, 0x1f, 5b6f2811172c96 Stephan Müller 2020-01-09  195 
> 			0x56, 0x49, 0x13, 0xc6, 0x46, 0x55, 0x73, 0x0d, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  196  			0x74, 0xcd, 0x81, 0xe0, 0x65, 
5b6f2811172c96
> Stephan Müller 2020-01-09  197  		}, {
> 5b6f2811172c96 Stephan Müller 2020-01-09  198  			0x4b, 0xf6, 
0x49, 0x89,
> 0x2a, 0x9f, 0x67, 0xd7, 5b6f2811172c96 Stephan Müller 2020-01-09  199 
> 			0xb8, 0x1d, 0xbb, 0x5d, 0xf0, 0x1b, 0x60, 0xb6, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  200  			0xb7, 0xf3, 0x86, 0x6d, 0xe0, 
0x04, 0xa1, 0xbc,
> 5b6f2811172c96 Stephan Müller 2020-01-09  201  			0x3b, 0xb0, 
0x10, 0x91,
> 0xe8, 0x22, 0x67, 0x5b, 5b6f2811172c96 Stephan Müller 2020-01-09  202 
> 			0xe8, 0xf0, 0x4f, 0x82, 0x70, 0xc7, 0xe1, 0xc8, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  203  			0xd8, 0xad, 0x70, 0xcf, 0xf6, 
5b6f2811172c96
> Stephan Müller 2020-01-09  204  		}, {
> 5b6f2811172c96 Stephan Müller 2020-01-09  205  			0x60, 0x1f, 
0x71, 0x07,
> 0x92, 0xae, 0xa0, 0x24, 5b6f2811172c96 Stephan Müller 2020-01-09  206 
> 			0xb6, 0xa4, 0x10, 0x70, 0x1f, 0x94, 0x51, 0x9a, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  207  			0x5a, 0x81, 0xc4, 0x46, 0x78, 
0x56, 0x71, 0xdd,
> 5b6f2811172c96 Stephan Müller 2020-01-09  208  			0x45, 0x63, 
0x01, 0x34,
> 0x87, 0x79, 0xb4, 0xd5, 5b6f2811172c96 Stephan Müller 2020-01-09  209 
> 			0x91, 0x79, 0xb9, 0x93, 0x11, 0x44, 0x50, 0xad, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  210  			0x64, 0x7e, 0x5c, 0xec, 0x16, 
5b6f2811172c96
> Stephan Müller 2020-01-09  211  		}, {
> 5b6f2811172c96 Stephan Müller 2020-01-09  212  			0x49, 0x2f, 
0xa0, 0x45,
> 0xf8, 0xb0, 0x80, 0x88, 5b6f2811172c96 Stephan Müller 2020-01-09  213 
> 			0x79, 0xeb, 0xb6, 0x82, 0x1c, 0xf3, 0x67, 0xc4, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  214  			0x88, 0x88, 0xe9, 0x75, 0x20, 
0x54, 0x78, 0xc6,
> 5b6f2811172c96 Stephan Müller 2020-01-09  215  			0x5c, 0x59, 
0xcf, 0xd9,
> 0x73, 0x12, 0x17, 0xf4, 5b6f2811172c96 Stephan Müller 2020-01-09  216 
> 			0x30, 0x9c, 0xb7, 0x21, 0x45, 0xe2, 0xb6, 0x0c, 
5b6f2811172c96 Stephan
> Müller 2020-01-09  217  			0x0c, 0xeb, 0x1b, 0xdc, 0xdc, 
5b6f2811172c96
> Stephan Müller 2020-01-09  218  		}
> 5b6f2811172c96 Stephan Müller 2020-01-09  219  	};
> 5b6f2811172c96 Stephan Müller 2020-01-09  220  	struct lrng_pool *lrng_pool,
> *lrng_pool_aligned; 5b6f2811172c96 Stephan Müller 2020-01-09  221  	u8
> hash_df[sizeof(lrng_hash_df_selftest_result[0])] 5b6f2811172c96 Stephan
> Müller 2020-01-09  222  							
__aligned(sizeof(u32)); 5b6f2811172c96
> Stephan Müller 2020-01-09  223  	u32 generated;
> 5b6f2811172c96 Stephan Müller 2020-01-09  224  	int ret = 0;
> 5b6f2811172c96 Stephan Müller 2020-01-09  225
> 5b6f2811172c96 Stephan Müller 2020-01-09  226 
> 	BUILD_BUG_ON(ARRAY_SIZE(lrng_hash_df_selftest_result) < 5b6f2811172c96
> Stephan Müller 2020-01-09  227  						
	CONFIG_LRNG_POOL_SIZE);
> 5b6f2811172c96 Stephan Müller 2020-01-09  228
> 5b6f2811172c96 Stephan Müller 2020-01-09 @229  	lrng_pool =
> kzalloc(sizeof(struct lrng_pool) + sizeof(LRNG_KCAPI_ALIGN),
> ^^^^^^^^^^^^^^^^^^^^^^^^ Here too.
> 
> 
> 5b6f2811172c96 Stephan Müller 2020-01-09  230  			    
GFP_KERNEL);
> 5b6f2811172c96 Stephan Müller 2020-01-09  231  	if (!lrng_pool)
> 5b6f2811172c96 Stephan Müller 2020-01-09  232  		return
> LRNG_SEFLTEST_ERROR_HASHDF; 5b6f2811172c96 Stephan Müller 2020-01-09  233 
> 	lrng_pool_aligned = PTR_ALIGN(lrng_pool, sizeof(u32)); 5b6f2811172c96
> Stephan Müller 2020-01-09  234
> 5b6f2811172c96 Stephan Müller 2020-01-09  235  	generated =
> __lrng_pool_hash_df(crypto_cb, NULL, lrng_pool_aligned, 5b6f2811172c96
> Stephan Müller 2020-01-09  236  					
hash_df, sizeof(hash_df) << 3);
> 5b6f2811172c96 Stephan Müller 2020-01-09  237
> 5b6f2811172c96 Stephan Müller 2020-01-09  238  	if ((generated >> 3) !=
> sizeof(hash_df) || 5b6f2811172c96 Stephan Müller 2020-01-09  239  	   
> memcmp(hash_df, lrng_hash_df_selftest_result[CONFIG_LRNG_POOL_SIZE],
> 5b6f2811172c96 Stephan Müller 2020-01-09  240  		   sizeof(hash_df))) 
{
> 5b6f2811172c96 Stephan Müller 2020-01-09  241  		pr_err("LRNG Hash DF
> self-test FAILED\n"); 5b6f2811172c96 Stephan Müller 2020-01-09  242  		
ret
> = LRNG_SEFLTEST_ERROR_HASHDF; 5b6f2811172c96 Stephan Müller 2020-01-09  243
>  	}
> 5b6f2811172c96 Stephan Müller 2020-01-09  244
> 5b6f2811172c96 Stephan Müller 2020-01-09  245  	kfree(lrng_pool);
> 5b6f2811172c96 Stephan Müller 2020-01-09  246  	return ret;
> 5b6f2811172c96 Stephan Müller 2020-01-09  247  }
> 
> ---
> 0-DAY kernel test infrastructure                 Open Source Technology
> Center https://lists.01.org/hyperkitty/list/kbuild-all-hn68Rpc1hR1g9hUCZPvPmw@public.gmane.org Intel
> Corporation



Ciao
Stephan

^ permalink raw reply

* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Al Viro @ 2020-01-13 13:30 UTC (permalink / raw)
  To: Ian Kent
  Cc: Aleksa Sarai, David Howells, Eric Biederman, Linus Torvalds,
	stable, Christian Brauner, Serge Hallyn, dev, containers,
	linux-api, linux-fsdevel, linux-kernel
In-Reply-To: <58f9894e51a00ad2a4ac3d4122bf29e7cb6c0d54.camel@themaw.net>

On Mon, Jan 13, 2020 at 02:03:00PM +0800, Ian Kent wrote:

> Oh wait, for systemd I was actually looking at:
> https://github.com/systemd/systemd/blob/master/src/shared/switch-root.c
> 
> > 
> > Mind you, that's not the actual systemd repo. either I probably
> > need to look a lot deeper (and at the actual systemd repo) to
> > work out what's actually being called.
> > 
> > > Sigh...  Guess I'll have to dig that Fedora KVM image out and
> > > try to see what it's about... ;-/  Here comes a couple of hours
> > > of build...

D'oh...  And yes, that would've been a bisect hazard - switch to
path_lookupat() later in the series gets rid of that.  Incremental
(to be foldede, of course):

diff --git a/fs/namei.c b/fs/namei.c
index 1793661c3342..204677c37751 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2634,7 +2634,7 @@ path_mountpoint(struct nameidata *nd, unsigned flags, struct path *path)
 		(err = lookup_last(nd)) > 0) {
 		s = trailing_symlink(nd);
 	}
-	if (!err)
+	if (!err && (nd->flags & LOOKUP_RCU))
 		err = unlazy_walk(nd);
 	if (!err)
 		err = handle_lookup_down(nd);

^ permalink raw reply related

* Re: [PATCH v9 2/4] pid: Implement pidfd_getfd syscall
From: Arnd Bergmann @ 2020-01-13 14:58 UTC (permalink / raw)
  To: Sargun Dhillon
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Linux Containers, Linux API, Linux FS-devel Mailing List,
	Tycho Andersen, Jann Horn, Aleksa Sarai, Christian Brauner,
	Oleg Nesterov, Andy Lutomirski, Al Viro, Gian-Carlo Pascutto,
	Emilio Cobos Álvarez, Florian Weimer, Jed Davis
In-Reply-To: <20200107175927.4558-3-sargun-GaZTRHToo+CzQB+pC5nmwQ@public.gmane.org>

On Tue, Jan 7, 2020 at 6:59 PM Sargun Dhillon <sargun-GaZTRHToo+CzQB+pC5nmwQ@public.gmane.org> wrote:
> +/**
> + * sys_pidfd_getfd() - Get a file descriptor from another process
> + *
> + * @pidfd:     the pidfd file descriptor of the process
> + * @fd:                the file descriptor number to get
> + * @flags:     flags on how to get the fd (reserved)
> + *
> + * This syscall gets a copy of a file descriptor from another process
> + * based on the pidfd, and file descriptor number. It requires that
> + * the calling process has the ability to ptrace the process represented
> + * by the pidfd. The process which is having its file descriptor copied
> + * is otherwise unaffected.
> + *
> + * Return: On success, a cloexec file descriptor is returned.
> + *         On error, a negative errno number will be returned.
> + */
> +SYSCALL_DEFINE3(pidfd_getfd, int, pidfd, int, fd,
> +               unsigned int, flags)

This is the most sensible definition I can see. I can not tell
whether we should or want to have it, but if everyone thinks
this is a good idea, then this ABI makes sense.

Reviewed-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

^ permalink raw reply

* Re: [PATCH v9 3/4] arch: wire up pidfd_getfd syscall
From: Arnd Bergmann @ 2020-01-13 14:59 UTC (permalink / raw)
  To: Sargun Dhillon
  Cc: linux-kernel@vger.kernel.org, Linux Containers, Linux API,
	Linux FS-devel Mailing List, Tycho Andersen, Jann Horn,
	Aleksa Sarai, Christian Brauner, Oleg Nesterov, Andy Lutomirski,
	Al Viro, Gian-Carlo Pascutto, Emilio Cobos Álvarez,
	Florian Weimer, Jed Davis
In-Reply-To: <20200107175927.4558-4-sargun@sargun.me>

On Tue, Jan 7, 2020 at 6:59 PM Sargun Dhillon <sargun@sargun.me> wrote:
>
> This wires up the pidfd_getfd syscall for all architectures.
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

This all looks correct,

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

^ permalink raw reply

* Re: [PATCH v9 1/4] vfs, fdtable: Add fget_task helper
From: Arnd Bergmann @ 2020-01-13 15:02 UTC (permalink / raw)
  To: Sargun Dhillon
  Cc: linux-kernel@vger.kernel.org, Linux Containers, Linux API,
	Linux FS-devel Mailing List, Tycho Andersen, Jann Horn,
	Aleksa Sarai, Christian Brauner, Oleg Nesterov, Andy Lutomirski,
	Al Viro, Gian-Carlo Pascutto, Emilio Cobos Álvarez,
	Florian Weimer, Jed Davis
In-Reply-To: <20200107175927.4558-2-sargun@sargun.me>

On Tue, Jan 7, 2020 at 6:59 PM Sargun Dhillon <sargun@sargun.me> wrote:
>
> This introduces a function which can be used to fetch a file, given an
> arbitrary task. As long as the user holds a reference (refcnt) to the
> task_struct it is safe to call, and will either return NULL on failure,
> or a pointer to the file, with a refcnt.
>
> This patch is based on Oleg Nesterov's (cf. [1]) patch from September
> 2018.
>
> [1]: Link: https://lore.kernel.org/r/20180915160423.GA31461@redhat.com
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> Suggested-by: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

^ permalink raw reply

* Re: [PATCH 2/4] mm: introduce external memory hinting API
From: Minchan Kim @ 2020-01-13 18:02 UTC (permalink / raw)
  To: SeongJae Park
  Cc: Andrew Morton, LKML, linux-mm, linux-api, oleksandr,
	Suren Baghdasaryan, Tim Murray, Daniel Colascione, Sandeep Patil,
	Sonny Rao, Brian Geffon, Michal Hocko, Johannes Weiner,
	Shakeel Butt, John Dias
In-Reply-To: <20200111073452.25182-1-sj38.park@gmail.com>

On Sat, Jan 11, 2020 at 08:34:52AM +0100, SeongJae Park wrote:
> On Fri, 10 Jan 2020 13:34:31 -0800 Minchan Kim <minchan@kernel.org> wrote:
> 
> > There are usecases that System Management Software(SMS) want to give
> > a memory hint to other processes because it's not known to the
> > application. In the case of Android, ActivityManagerService daemon
> > manges app's life cycle and that daemon must be able to initiate
> > reclaim on its own without any app involvement.
> > 
> > To solve the issue, this patch introduces new syscall process_madvise(2).
> > It uses pidfd of an external processs to give the hint.
> > 
> >  int process_madvise(int pidfd, void *addr, size_t length, int advise,
> > 			unsigned long flag);
> > 
> > Since it could affect other process's address range, only privileged
> > process(CAP_SYS_PTRACE) or something else(e.g., being the same UID)
> > gives it the right to ptrace the process could use it successfully.
> > The flag argument is reserved for future use if we need to extend the
> > API.
> > 
> > Supporting all hints madvise has/will supported/support to process_madvise
> > is rather risky. Because we are not sure all hints make sense from external
> > process and implementation for the hint may rely on the caller being
> > in the current context so it could be error-prone. Thus, I just limited
> > hints as MADV_[COLD|PAGEOUT] in this patch.
> > 
> > If someone want to add other hints, we could hear hear the usecase and
> > review it for each hint. It's more safe for maintainace rather than
> > introducing a buggy syscall but hard to fix it later.
> > 
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  arch/alpha/kernel/syscalls/syscall.tbl      |  1 +
> >  arch/arm/tools/syscall.tbl                  |  1 +
> >  arch/arm64/include/asm/unistd.h             |  2 +-
> >  arch/arm64/include/asm/unistd32.h           |  2 +
> >  arch/ia64/kernel/syscalls/syscall.tbl       |  1 +
> >  arch/m68k/kernel/syscalls/syscall.tbl       |  1 +
> >  arch/microblaze/kernel/syscalls/syscall.tbl |  1 +
> >  arch/mips/kernel/syscalls/syscall_n32.tbl   |  1 +
> >  arch/mips/kernel/syscalls/syscall_n64.tbl   |  1 +
> >  arch/parisc/kernel/syscalls/syscall.tbl     |  1 +
> >  arch/powerpc/kernel/syscalls/syscall.tbl    |  1 +
> >  arch/s390/kernel/syscalls/syscall.tbl       |  1 +
> >  arch/sh/kernel/syscalls/syscall.tbl         |  1 +
> >  arch/sparc/kernel/syscalls/syscall.tbl      |  1 +
> >  arch/x86/entry/syscalls/syscall_32.tbl      |  1 +
> >  arch/x86/entry/syscalls/syscall_64.tbl      |  1 +
> >  arch/xtensa/kernel/syscalls/syscall.tbl     |  1 +
> >  include/linux/syscalls.h                    |  2 +
> >  include/uapi/asm-generic/unistd.h           |  5 +-
> >  kernel/sys_ni.c                             |  1 +
> >  mm/madvise.c                                | 64 +++++++++++++++++++++
> >  21 files changed, 89 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
> > index e56950f23b49..776c61803315 100644
> > --- a/arch/alpha/kernel/syscalls/syscall.tbl
> > +++ b/arch/alpha/kernel/syscalls/syscall.tbl
> > @@ -477,3 +477,4 @@
> >  # 545 reserved for clone3
> >  546	common	watch_devices			sys_watch_devices
> >  547	common	openat2				sys_openat2
> > +548	common	process_madvise			sys_process_madvise
> > diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
> > index 7fb2f4d59210..a43381542276 100644
> > --- a/arch/arm/tools/syscall.tbl
> > +++ b/arch/arm/tools/syscall.tbl
> > @@ -451,3 +451,4 @@
> >  435	common	clone3				sys_clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +438	common	process_madvise			sys_process_madvise
> > diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
> > index 8aa00ccb0b96..b722e47377a5 100644
> > --- a/arch/arm64/include/asm/unistd.h
> > +++ b/arch/arm64/include/asm/unistd.h
> > @@ -38,7 +38,7 @@
> >  #define __ARM_NR_compat_set_tls		(__ARM_NR_COMPAT_BASE + 5)
> >  #define __ARM_NR_COMPAT_END		(__ARM_NR_COMPAT_BASE + 0x800)
> >  
> > -#define __NR_compat_syscalls		438
> > +#define __NR_compat_syscalls		439
> >  #endif
> >  
> >  #define __ARCH_WANT_SYS_CLONE
> > diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
> > index 31f0ce25719e..5c82557d408f 100644
> > --- a/arch/arm64/include/asm/unistd32.h
> > +++ b/arch/arm64/include/asm/unistd32.h
> > @@ -883,6 +883,8 @@ __SYSCALL(__NR_clone3, sys_clone3)
> >  __SYSCALL(__NR_watch_devices, sys_watch_devices)
> >  #define __NR_openat2 437
> >  __SYSCALL(__NR_openat2, sys_openat2)
> > +#define __NR_openat2 438
> 
> Shouldn't this be '#define __NR_process_madvise 438'?
> 
> > +__SYSCALL(__NR_process_madvise, process_madvise)
> >  
> >  /*
> >   * Please add new compat syscalls above this comment and update
> > diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl
> > index b9aa59931905..c156abc9a298 100644
> > --- a/arch/ia64/kernel/syscalls/syscall.tbl
> > +++ b/arch/ia64/kernel/syscalls/syscall.tbl
> > @@ -358,3 +358,4 @@
> >  # 435 reserved for clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +438	common	process_madvise			sys_process_madvise
> > diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
> > index 868c1ef89d35..5b6034b6650f 100644
> > --- a/arch/m68k/kernel/syscalls/syscall.tbl
> > +++ b/arch/m68k/kernel/syscalls/syscall.tbl
> > @@ -437,3 +437,4 @@
> >  # 435 reserved for clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +438	common	process_madvise			sys_process_madvise
> > diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
> > index 544b4cef18b3..4bef584af09c 100644
> > --- a/arch/microblaze/kernel/syscalls/syscall.tbl
> > +++ b/arch/microblaze/kernel/syscalls/syscall.tbl
> > @@ -443,3 +443,4 @@
> >  435	common	clone3				sys_clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +438	common	process_madvise			sys_process_madvise
> > diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
> > index 05e8aee5dae7..94fbd0fcccce 100644
> > --- a/arch/mips/kernel/syscalls/syscall_n32.tbl
> > +++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
> > @@ -376,3 +376,4 @@
> >  435	n32	clone3				__sys_clone3
> >  436	n32	watch_devices			sys_watch_devices
> >  437	n32	openat2				sys_openat2
> > +437	n32	process_madivse			sys_process_madvise
> 
> Shouldn't the number for the 'process_madvise' be '438' instead of '437'?
> 
> > diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
> > index 24d6c01328fb..4e6982c429d5 100644
> > --- a/arch/mips/kernel/syscalls/syscall_n64.tbl
> > +++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
> > @@ -352,3 +352,4 @@
> >  435	n64	clone3				__sys_clone3
> >  436	n64	watch_devices			sys_watch_devices
> >  437	n64	openat2				sys_openat2
> > +437	n64	process_madvise			sys_process_madvise
> 
> 438?  Same for below 5 changes.
> 
> > diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
> > index 4b5f77a4e1a2..3aa990caf9dc 100644
> > --- a/arch/parisc/kernel/syscalls/syscall.tbl
> > +++ b/arch/parisc/kernel/syscalls/syscall.tbl
> > @@ -435,3 +435,4 @@
> >  435	common	clone3				sys_clone3_wrapper
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +437	common	process_madvise			sys_process_madvise
> > diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
> > index 9716dc85a517..30e727a23f33 100644
> > --- a/arch/powerpc/kernel/syscalls/syscall.tbl
> > +++ b/arch/powerpc/kernel/syscalls/syscall.tbl
> > @@ -519,3 +519,4 @@
> >  435	nospu	clone3				ppc_clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +437	common	process_madvise			sys_process_madvise
> > diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
> > index 7da330f8b03e..75722e5ff496 100644
> > --- a/arch/s390/kernel/syscalls/syscall.tbl
> > +++ b/arch/s390/kernel/syscalls/syscall.tbl
> > @@ -440,3 +440,4 @@
> >  435  common	clone3			sys_clone3			sys_clone3
> >  436  common	watch_devices		sys_watch_devices		sys_watch_devices
> >  437  common	openat2			sys_openat2			sys_openat2
> > +437  common	process_madvise		sys_process_madvise		sys_process_madvise
> > diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
> > index bb7e68e25337..7d7bc7befad3 100644
> > --- a/arch/sh/kernel/syscalls/syscall.tbl
> > +++ b/arch/sh/kernel/syscalls/syscall.tbl
> > @@ -440,3 +440,4 @@
> >  # 435 reserved for clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +437	common	process_madvise			sys_process_madvise
> > diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
> > index 646a1fad7218..581d331ff62f 100644
> > --- a/arch/sparc/kernel/syscalls/syscall.tbl
> > +++ b/arch/sparc/kernel/syscalls/syscall.tbl
> > @@ -483,3 +483,4 @@
> >  # 435 reserved for clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2			sys_openat2
> > +437	common	process_madvise		sys_process_madvise
> > diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
> > index 57c53acee290..76a2c266fe7e 100644
> > --- a/arch/x86/entry/syscalls/syscall_32.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_32.tbl
> > @@ -442,3 +442,4 @@
> >  435	i386	clone3			sys_clone3			__ia32_sys_clone3
> >  436	i386	watch_devices		sys_watch_devices		__ia32_sys_watch_devices
> >  437	i386	openat2			sys_openat2			__ia32_sys_openat2
> > +438	i386	process_madvise		sys_process_madvise		__ia32_sys_process_madvise
> > diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
> > index 1dd8d21f6500..b697cd8620cb 100644
> > --- a/arch/x86/entry/syscalls/syscall_64.tbl
> > +++ b/arch/x86/entry/syscalls/syscall_64.tbl
> > @@ -359,6 +359,7 @@
> >  435	common	clone3			__x64_sys_clone3/ptregs
> >  436	common	watch_devices		__x64_sys_watch_devices
> >  437	common	openat2			__x64_sys_openat2
> > +438	common	process_madvise		__x64_sys_process_madvise
> >  
> >  #
> >  # x32-specific system call numbers start at 512 to avoid cache impact
> > diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
> > index 0f48ab7bd75b..2e9813ecfd7d 100644
> > --- a/arch/xtensa/kernel/syscalls/syscall.tbl
> > +++ b/arch/xtensa/kernel/syscalls/syscall.tbl
> > @@ -408,3 +408,4 @@
> >  435	common	clone3				sys_clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +438	common	process_madvise			sys_process_madvise
> > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> > index 433c8c85636e..1b58a11ff49f 100644
> > --- a/include/linux/syscalls.h
> > +++ b/include/linux/syscalls.h
> > @@ -877,6 +877,8 @@ asmlinkage long sys_munlockall(void);
> >  asmlinkage long sys_mincore(unsigned long start, size_t len,
> >  				unsigned char __user * vec);
> >  asmlinkage long sys_madvise(unsigned long start, size_t len, int behavior);
> > +asmlinkage long sys_process_madvise(int pidfd, unsigned long start,
> > +			size_t len, int behavior, unsigned long flags);
> >  asmlinkage long sys_remap_file_pages(unsigned long start, unsigned long size,
> >  			unsigned long prot, unsigned long pgoff,
> >  			unsigned long flags);
> > diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
> > index 33f3856a9c3c..4bcd8d366f38 100644
> > --- a/include/uapi/asm-generic/unistd.h
> > +++ b/include/uapi/asm-generic/unistd.h
> > @@ -856,8 +856,11 @@ __SYSCALL(__NR_watch_devices, sys_watch_devices)
> >  #define __NR_openat2 437
> >  __SYSCALL(__NR_openat2, sys_openat2)
> >  
> > +#define __NR_openat2 438
> 
> Shouldn't this be '#define __NR_process_madvise 438'?
> 

Hi SeongJae,

I fixed all you pointed out.

Thanks for the review.

^ permalink raw reply

* Re: [PATCH 1/4] mm: factor out madvise's core functionality
From: Minchan Kim @ 2020-01-13 18:11 UTC (permalink / raw)
  To: SeongJae Park
  Cc: Andrew Morton, LKML, linux-mm, linux-api, oleksandr,
	Suren Baghdasaryan, Tim Murray, Daniel Colascione, Sandeep Patil,
	Sonny Rao, Brian Geffon, Michal Hocko, Johannes Weiner,
	Shakeel Butt, John Dias
In-Reply-To: <20200111073737.25321-1-sj38.park@gmail.com>

On Sat, Jan 11, 2020 at 08:37:37AM +0100, SeongJae Park wrote:
> On Fri, 10 Jan 2020 13:34:30 -0800 Minchan Kim <minchan@kernel.org> wrote:
> 
> > This patch factor out madvise's core functionality so that upcoming
> > patch can reuse it without duplication. It shouldn't change any behavior.
> > 
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  mm/madvise.c | 194 +++++++++++++++++++++++++++++----------------------
> >  1 file changed, 111 insertions(+), 83 deletions(-)
> > 
> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index bcdb6a042787..0c901de531e4 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -35,6 +35,7 @@
> >  struct madvise_walk_private {
> >  	struct mmu_gather *tlb;
> >  	bool pageout;
> > +	struct task_struct *task;
> >  };
> >  
> >  /*
> > @@ -306,12 +307,13 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
> >  	bool pageout = private->pageout;
> >  	struct mm_struct *mm = tlb->mm;
> >  	struct vm_area_struct *vma = walk->vma;
> > +	struct task_struct *task = private->task;
> >  	pte_t *orig_pte, *pte, ptent;
> >  	spinlock_t *ptl;
> >  	struct page *page = NULL;
> >  	LIST_HEAD(page_list);
> >  
> > -	if (fatal_signal_pending(current))
> > +	if (fatal_signal_pending(task))
> >  		return -EINTR;
> >  
> >  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> > @@ -469,12 +471,14 @@ static const struct mm_walk_ops cold_walk_ops = {
> >  };
> >  
> >  static void madvise_cold_page_range(struct mmu_gather *tlb,
> > +			     struct task_struct *task,
> >  			     struct vm_area_struct *vma,
> >  			     unsigned long addr, unsigned long end)
> >  {
> >  	struct madvise_walk_private walk_private = {
> >  		.pageout = false,
> >  		.tlb = tlb,
> > +		.task = task,
> >  	};
> >  
> >  	tlb_start_vma(tlb, vma);
> > @@ -482,7 +486,7 @@ static void madvise_cold_page_range(struct mmu_gather *tlb,
> >  	tlb_end_vma(tlb, vma);
> >  }
> >  
> > -static long madvise_cold(struct vm_area_struct *vma,
> > +static long madvise_cold(struct task_struct *task, struct vm_area_struct *vma,
> >  			struct vm_area_struct **prev,
> >  			unsigned long start_addr, unsigned long end_addr)
> >  {
> > @@ -495,19 +499,21 @@ static long madvise_cold(struct vm_area_struct *vma,
> >  
> >  	lru_add_drain();
> >  	tlb_gather_mmu(&tlb, mm, start_addr, end_addr);
> > -	madvise_cold_page_range(&tlb, vma, start_addr, end_addr);
> > +	madvise_cold_page_range(&tlb, task, vma, start_addr, end_addr);
> >  	tlb_finish_mmu(&tlb, start_addr, end_addr);
> >  
> >  	return 0;
> >  }
> >  
> >  static void madvise_pageout_page_range(struct mmu_gather *tlb,
> > +			     struct task_struct *task,
> >  			     struct vm_area_struct *vma,
> >  			     unsigned long addr, unsigned long end)
> >  {
> >  	struct madvise_walk_private walk_private = {
> >  		.pageout = true,
> >  		.tlb = tlb,
> > +		.task = task,
> >  	};
> >  
> >  	tlb_start_vma(tlb, vma);
> > @@ -531,9 +537,9 @@ static inline bool can_do_pageout(struct vm_area_struct *vma)
> >  		inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0;
> >  }
> >  
> > -static long madvise_pageout(struct vm_area_struct *vma,
> > -			struct vm_area_struct **prev,
> > -			unsigned long start_addr, unsigned long end_addr)
> > +static long madvise_pageout(struct task_struct *task,
> > +		struct vm_area_struct *vma, struct vm_area_struct **prev,
> > +		unsigned long start_addr, unsigned long end_addr)
> >  {
> >  	struct mm_struct *mm = vma->vm_mm;
> >  	struct mmu_gather tlb;
> > @@ -547,7 +553,7 @@ static long madvise_pageout(struct vm_area_struct *vma,
> >  
> >  	lru_add_drain();
> >  	tlb_gather_mmu(&tlb, mm, start_addr, end_addr);
> > -	madvise_pageout_page_range(&tlb, vma, start_addr, end_addr);
> > +	madvise_pageout_page_range(&tlb, task, vma, start_addr, end_addr);
> >  	tlb_finish_mmu(&tlb, start_addr, end_addr);
> >  
> >  	return 0;
> > @@ -751,7 +757,8 @@ static long madvise_dontneed_single_vma(struct vm_area_struct *vma,
> >  	return 0;
> >  }
> >  
> > -static long madvise_dontneed_free(struct vm_area_struct *vma,
> > +static long madvise_dontneed_free(struct mm_struct *mm,
> > +				  struct vm_area_struct *vma,
> >  				  struct vm_area_struct **prev,
> >  				  unsigned long start, unsigned long end,
> >  				  int behavior)
> > @@ -763,8 +770,8 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
> >  	if (!userfaultfd_remove(vma, start, end)) {
> >  		*prev = NULL; /* mmap_sem has been dropped, prev is stale */
> >  
> > -		down_read(&current->mm->mmap_sem);
> > -		vma = find_vma(current->mm, start);
> > +		down_read(&mm->mmap_sem);
> > +		vma = find_vma(mm, start);
> >  		if (!vma)
> >  			return -ENOMEM;
> >  		if (start < vma->vm_start) {
> > @@ -811,7 +818,8 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
> >   * Application wants to free up the pages and associated backing store.
> >   * This is effectively punching a hole into the middle of a file.
> >   */
> > -static long madvise_remove(struct vm_area_struct *vma,
> > +static long madvise_remove(struct mm_struct *mm,
> > +				struct vm_area_struct *vma,
> >  				struct vm_area_struct **prev,
> >  				unsigned long start, unsigned long end)
> >  {
> > @@ -845,13 +853,13 @@ static long madvise_remove(struct vm_area_struct *vma,
> >  	get_file(f);
> >  	if (userfaultfd_remove(vma, start, end)) {
> >  		/* mmap_sem was not released by userfaultfd_remove() */
> > -		up_read(&current->mm->mmap_sem);
> > +		up_read(&mm->mmap_sem);
> >  	}
> >  	error = vfs_fallocate(f,
> >  				FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> >  				offset, end - start);
> >  	fput(f);
> > -	down_read(&current->mm->mmap_sem);
> > +	down_read(&mm->mmap_sem);
> >  	return error;
> >  }
> >  
> > @@ -925,21 +933,23 @@ static int madvise_inject_error(int behavior,
> >  #endif
> >  
> >  static long
> > -madvise_vma(struct vm_area_struct *vma, struct vm_area_struct **prev,
> > +madvise_vma(struct task_struct *task, struct mm_struct *mm,
> > +		struct vm_area_struct *vma, struct vm_area_struct **prev,
> >  		unsigned long start, unsigned long end, int behavior)
> >  {
> >  	switch (behavior) {
> >  	case MADV_REMOVE:
> > -		return madvise_remove(vma, prev, start, end);
> > +		return madvise_remove(mm, vma, prev, start, end);
> >  	case MADV_WILLNEED:
> >  		return madvise_willneed(vma, prev, start, end);
> >  	case MADV_COLD:
> > -		return madvise_cold(vma, prev, start, end);
> > +		return madvise_cold(task, vma, prev, start, end);
> >  	case MADV_PAGEOUT:
> > -		return madvise_pageout(vma, prev, start, end);
> > +		return madvise_pageout(task, vma, prev, start, end);
> >  	case MADV_FREE:
> >  	case MADV_DONTNEED:
> > -		return madvise_dontneed_free(vma, prev, start, end, behavior);
> > +		return madvise_dontneed_free(mm, vma, prev, start,
> > +						end, behavior);
> >  	default:
> >  		return madvise_behavior(vma, prev, start, end, behavior);
> >  	}
> > @@ -984,67 +994,19 @@ madvise_behavior_valid(int behavior)
> >  }
> >  
> >  /*
> > - * The madvise(2) system call.
> > + * madvise_common - request behavior hint to address range of the target process
> >   *
> > - * Applications can use madvise() to advise the kernel how it should
> > - * handle paging I/O in this VM area.  The idea is to help the kernel
> > - * use appropriate read-ahead and caching techniques.  The information
> > - * provided is advisory only, and can be safely disregarded by the
> > - * kernel without affecting the correct operation of the application.
> > + * @task: task_struct got behavior hint, not giving the hint
> > + * @mm: mm_struct got behavior hint, not giving the hint
> > + * @start: base address of the hinted range
> > + * @len_in: length of the hinted range
> > + * @behavior: requested hint
> >   *
> > - * behavior values:
> > - *  MADV_NORMAL - the default behavior is to read clusters.  This
> > - *		results in some read-ahead and read-behind.
> > - *  MADV_RANDOM - the system should read the minimum amount of data
> > - *		on any access, since it is unlikely that the appli-
> > - *		cation will need more than what it asks for.
> > - *  MADV_SEQUENTIAL - pages in the given range will probably be accessed
> > - *		once, so they can be aggressively read ahead, and
> > - *		can be freed soon after they are accessed.
> > - *  MADV_WILLNEED - the application is notifying the system to read
> > - *		some pages ahead.
> > - *  MADV_DONTNEED - the application is finished with the given range,
> > - *		so the kernel can free resources associated with it.
> > - *  MADV_FREE - the application marks pages in the given range as lazy free,
> > - *		where actual purges are postponed until memory pressure happens.
> > - *  MADV_REMOVE - the application wants to free up the given range of
> > - *		pages and associated backing store.
> > - *  MADV_DONTFORK - omit this area from child's address space when forking:
> > - *		typically, to avoid COWing pages pinned by get_user_pages().
> > - *  MADV_DOFORK - cancel MADV_DONTFORK: no longer omit this area when forking.
> > - *  MADV_WIPEONFORK - present the child process with zero-filled memory in this
> > - *              range after a fork.
> > - *  MADV_KEEPONFORK - undo the effect of MADV_WIPEONFORK
> > - *  MADV_HWPOISON - trigger memory error handler as if the given memory range
> > - *		were corrupted by unrecoverable hardware memory failure.
> > - *  MADV_SOFT_OFFLINE - try to soft-offline the given range of memory.
> > - *  MADV_MERGEABLE - the application recommends that KSM try to merge pages in
> > - *		this area with pages of identical content from other such areas.
> > - *  MADV_UNMERGEABLE- cancel MADV_MERGEABLE: no longer merge pages with others.
> > - *  MADV_HUGEPAGE - the application wants to back the given range by transparent
> > - *		huge pages in the future. Existing pages might be coalesced and
> > - *		new pages might be allocated as THP.
> > - *  MADV_NOHUGEPAGE - mark the given range as not worth being backed by
> > - *		transparent huge pages so the existing pages will not be
> > - *		coalesced into THP and new pages will not be allocated as THP.
> > - *  MADV_DONTDUMP - the application wants to prevent pages in the given range
> > - *		from being included in its core dump.
> > - *  MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump.
> > - *
> > - * return values:
> > - *  zero    - success
> > - *  -EINVAL - start + len < 0, start is not page-aligned,
> > - *		"behavior" is not a valid value, or application
> > - *		is attempting to release locked or shared pages,
> > - *		or the specified address range includes file, Huge TLB,
> > - *		MAP_SHARED or VMPFNMAP range.
> > - *  -ENOMEM - addresses in the specified range are not currently
> > - *		mapped, or are outside the AS of the process.
> > - *  -EIO    - an I/O error occurred while paging in data.
> > - *  -EBADF  - map exists, but area maps something that isn't a file.
> > - *  -EAGAIN - a kernel resource was temporarily unavailable.
> > + * @task could be a zombie leader if it calls sys_exit so accessing mm_struct
> > + * via task->mm is prohibited. Please use @mm instead of task->mm.
> >   */
> > -SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> > +static int madvise_common(struct task_struct *task, struct mm_struct *mm,
> > +			unsigned long start, size_t len_in, int behavior)
> >  {
> >  	unsigned long end, tmp;
> >  	struct vm_area_struct *vma, *prev;
> > @@ -1082,10 +1044,10 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> >  
> >  	write = madvise_need_mmap_write(behavior);
> >  	if (write) {
> > -		if (down_write_killable(&current->mm->mmap_sem))
> > +		if (down_write_killable(&mm->mmap_sem))
> >  			return -EINTR;
> >  	} else {
> > -		down_read(&current->mm->mmap_sem);
> > +		down_read(&mm->mmap_sem);
> >  	}
> >  
> >  	/*
> > @@ -1093,7 +1055,7 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> >  	 * ranges, just ignore them, but return -ENOMEM at the end.
> >  	 * - different from the way of handling in mlock etc.
> >  	 */
> > -	vma = find_vma_prev(current->mm, start, &prev);
> > +	vma = find_vma_prev(mm, start, &prev);
> >  	if (vma && start > vma->vm_start)
> >  		prev = vma;
> >  
> > @@ -1118,7 +1080,7 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> >  			tmp = end;
> >  
> >  		/* Here vma->vm_start <= start < tmp <= (end|vma->vm_end). */
> > -		error = madvise_vma(vma, &prev, start, tmp, behavior);
> > +		error = madvise_vma(task, mm, vma, &prev, start, tmp, behavior);
> >  		if (error)
> >  			goto out;
> >  		start = tmp;
> > @@ -1130,14 +1092,80 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> >  		if (prev)
> >  			vma = prev->vm_next;
> >  		else	/* madvise_remove dropped mmap_sem */
> > -			vma = find_vma(current->mm, start);
> > +			vma = find_vma(mm, start);
> >  	}
> >  out:
> >  	blk_finish_plug(&plug);
> >  	if (write)
> > -		up_write(&current->mm->mmap_sem);
> > +		up_write(&mm->mmap_sem);
> >  	else
> > -		up_read(&current->mm->mmap_sem);
> > +		up_read(&mm->mmap_sem);
> >  
> >  	return error;
> >  }
> > +
> > +/*
> > + * The madvise(2) system call.
> > + *
> > + * Applications can use madvise() to advise the kernel how it should
> > + * handle paging I/O in this VM area.  The idea is to help the kernel
> > + * use appropriate read-ahead and caching techniques.  The information
> > + * provided is advisory only, and can be safely disregarded by the
> > + * kernel without affecting the correct operation of the application.
> > + *
> > + * behavior values:
> > + *  MADV_NORMAL - the default behavior is to read clusters.  This
> > + *		results in some read-ahead and read-behind.
> > + *  MADV_RANDOM - the system should read the minimum amount of data
> > + *		on any access, since it is unlikely that the appli-
> > + *		cation will need more than what it asks for.
> > + *  MADV_SEQUENTIAL - pages in the given range will probably be accessed
> > + *		once, so they can be aggressively read ahead, and
> > + *		can be freed soon after they are accessed.
> > + *  MADV_WILLNEED - the application is notifying the system to read
> > + *		some pages ahead.
> > + *  MADV_DONTNEED - the application is finished with the given range,
> > + *		so the kernel can free resources associated with it.
> > + *  MADV_FREE - the application marks pages in the given range as lazy free,
> > + *		where actual purges are postponed until memory pressure happens.
> > + *  MADV_REMOVE - the application wants to free up the given range of
> > + *		pages and associated backing store.
> > + *  MADV_DONTFORK - omit this area from child's address space when forking:
> > + *		typically, to avoid COWing pages pinned by get_user_pages().
> > + *  MADV_DOFORK - cancel MADV_DONTFORK: no longer omit this area when forking.
> > + *  MADV_WIPEONFORK - present the child process with zero-filled memory in this
> > + *              range after a fork.
> > + *  MADV_KEEPONFORK - undo the effect of MADV_WIPEONFORK
> > + *  MADV_HWPOISON - trigger memory error handler as if the given memory range
> > + *		were corrupted by unrecoverable hardware memory failure.
> > + *  MADV_SOFT_OFFLINE - try to soft-offline the given range of memory.
> > + *  MADV_MERGEABLE - the application recommends that KSM try to merge pages in
> > + *		this area with pages of identical content from other such areas.
> > + *  MADV_UNMERGEABLE- cancel MADV_MERGEABLE: no longer merge pages with others.
> > + *  MADV_HUGEPAGE - the application wants to back the given range by transparent
> > + *		huge pages in the future. Existing pages might be coalesced and
> > + *		new pages might be allocated as THP.
> > + *  MADV_NOHUGEPAGE - mark the given range as not worth being backed by
> > + *		transparent huge pages so the existing pages will not be
> > + *		coalesced into THP and new pages will not be allocated as THP.
> > + *  MADV_DONTDUMP - the application wants to prevent pages in the given range
> > + *		from being included in its core dump.
> > + *  MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump.
> 
> Just a trivial suggestion.  How about adding brief descriptions for the
> 'MADV_COLD' and 'MADV_PAGEOUT' here, probably with another patch?

How about this?
Feel free to suggest better wording.

diff --git a/mm/madvise.c b/mm/madvise.c
index 3aa9aec6bfd9..78b3ab789486 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1169,6 +1169,10 @@ static int madvise_common(struct task_struct *task, struct mm_struct *mm,
  *  MADV_DONTDUMP - the application wants to prevent pages in the given range
  *		from being included in its core dump.
  *  MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump.
+ *  MADV_COLD - the application uses the memory less so the kernel can deactivate
+ *  		the memory to evict them quickly when the memory pressure happen.
+ *  MADV_PAGEOUT - the application uses the memroy very rarely so kernel can
+ *  		page out the memory instantly.
  *
  * return values:
  *  zero    - success

Thanks.

^ permalink raw reply related

* Re: Re: [PATCH 1/4] mm: factor out madvise's core functionality
From: SeongJae Park @ 2020-01-13 18:22 UTC (permalink / raw)
  To: Minchan Kim
  Cc: SeongJae Park, Andrew Morton, LKML, linux-mm, linux-api,
	oleksandr, Suren Baghdasaryan, Tim Murray, Daniel Colascione,
	Sandeep Patil, Sonny Rao, Brian Geffon, Michal Hocko,
	Johannes Weiner, Shakeel Butt, John Dias
In-Reply-To: <20200113181118.GB110363@google.com>

On Mon, 13 Jan 2020 10:11:18 -0800 Minchan Kim <minchan@kernel.org> wrote:

> On Sat, Jan 11, 2020 at 08:37:37AM +0100, SeongJae Park wrote:
> > On Fri, 10 Jan 2020 13:34:30 -0800 Minchan Kim <minchan@kernel.org> wrote:
> > 
> > > This patch factor out madvise's core functionality so that upcoming
> > > patch can reuse it without duplication. It shouldn't change any behavior.
> > > 
> > > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > > ---
> > >  mm/madvise.c | 194 +++++++++++++++++++++++++++++----------------------
> > >  1 file changed, 111 insertions(+), 83 deletions(-)
> > > 
> > > diff --git a/mm/madvise.c b/mm/madvise.c
> > > index bcdb6a042787..0c901de531e4 100644
> > > --- a/mm/madvise.c
> > > +++ b/mm/madvise.c
> > > @@ -35,6 +35,7 @@
> > >  struct madvise_walk_private {
> > >  	struct mmu_gather *tlb;
> > >  	bool pageout;
> > > +	struct task_struct *task;
> > >  };
> > >  
> > >  /*
> > > @@ -306,12 +307,13 @@ static int madvise_cold_or_pageout_pte_range(pmd_t *pmd,
> > >  	bool pageout = private->pageout;
> > >  	struct mm_struct *mm = tlb->mm;
> > >  	struct vm_area_struct *vma = walk->vma;
> > > +	struct task_struct *task = private->task;
> > >  	pte_t *orig_pte, *pte, ptent;
> > >  	spinlock_t *ptl;
> > >  	struct page *page = NULL;
> > >  	LIST_HEAD(page_list);
> > >  
> > > -	if (fatal_signal_pending(current))
> > > +	if (fatal_signal_pending(task))
> > >  		return -EINTR;
> > >  
> > >  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> > > @@ -469,12 +471,14 @@ static const struct mm_walk_ops cold_walk_ops = {
> > >  };
> > >  
> > >  static void madvise_cold_page_range(struct mmu_gather *tlb,
> > > +			     struct task_struct *task,
> > >  			     struct vm_area_struct *vma,
> > >  			     unsigned long addr, unsigned long end)
> > >  {
> > >  	struct madvise_walk_private walk_private = {
> > >  		.pageout = false,
> > >  		.tlb = tlb,
> > > +		.task = task,
> > >  	};
> > >  
> > >  	tlb_start_vma(tlb, vma);
> > > @@ -482,7 +486,7 @@ static void madvise_cold_page_range(struct mmu_gather *tlb,
> > >  	tlb_end_vma(tlb, vma);
> > >  }
> > >  
> > > -static long madvise_cold(struct vm_area_struct *vma,
> > > +static long madvise_cold(struct task_struct *task, struct vm_area_struct *vma,
> > >  			struct vm_area_struct **prev,
> > >  			unsigned long start_addr, unsigned long end_addr)
> > >  {
> > > @@ -495,19 +499,21 @@ static long madvise_cold(struct vm_area_struct *vma,
> > >  
> > >  	lru_add_drain();
> > >  	tlb_gather_mmu(&tlb, mm, start_addr, end_addr);
> > > -	madvise_cold_page_range(&tlb, vma, start_addr, end_addr);
> > > +	madvise_cold_page_range(&tlb, task, vma, start_addr, end_addr);
> > >  	tlb_finish_mmu(&tlb, start_addr, end_addr);
> > >  
> > >  	return 0;
> > >  }
> > >  
> > >  static void madvise_pageout_page_range(struct mmu_gather *tlb,
> > > +			     struct task_struct *task,
> > >  			     struct vm_area_struct *vma,
> > >  			     unsigned long addr, unsigned long end)
> > >  {
> > >  	struct madvise_walk_private walk_private = {
> > >  		.pageout = true,
> > >  		.tlb = tlb,
> > > +		.task = task,
> > >  	};
> > >  
> > >  	tlb_start_vma(tlb, vma);
> > > @@ -531,9 +537,9 @@ static inline bool can_do_pageout(struct vm_area_struct *vma)
> > >  		inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0;
> > >  }
> > >  
> > > -static long madvise_pageout(struct vm_area_struct *vma,
> > > -			struct vm_area_struct **prev,
> > > -			unsigned long start_addr, unsigned long end_addr)
> > > +static long madvise_pageout(struct task_struct *task,
> > > +		struct vm_area_struct *vma, struct vm_area_struct **prev,
> > > +		unsigned long start_addr, unsigned long end_addr)
> > >  {
> > >  	struct mm_struct *mm = vma->vm_mm;
> > >  	struct mmu_gather tlb;
> > > @@ -547,7 +553,7 @@ static long madvise_pageout(struct vm_area_struct *vma,
> > >  
> > >  	lru_add_drain();
> > >  	tlb_gather_mmu(&tlb, mm, start_addr, end_addr);
> > > -	madvise_pageout_page_range(&tlb, vma, start_addr, end_addr);
> > > +	madvise_pageout_page_range(&tlb, task, vma, start_addr, end_addr);
> > >  	tlb_finish_mmu(&tlb, start_addr, end_addr);
> > >  
> > >  	return 0;
> > > @@ -751,7 +757,8 @@ static long madvise_dontneed_single_vma(struct vm_area_struct *vma,
> > >  	return 0;
> > >  }
> > >  
> > > -static long madvise_dontneed_free(struct vm_area_struct *vma,
> > > +static long madvise_dontneed_free(struct mm_struct *mm,
> > > +				  struct vm_area_struct *vma,
> > >  				  struct vm_area_struct **prev,
> > >  				  unsigned long start, unsigned long end,
> > >  				  int behavior)
> > > @@ -763,8 +770,8 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
> > >  	if (!userfaultfd_remove(vma, start, end)) {
> > >  		*prev = NULL; /* mmap_sem has been dropped, prev is stale */
> > >  
> > > -		down_read(&current->mm->mmap_sem);
> > > -		vma = find_vma(current->mm, start);
> > > +		down_read(&mm->mmap_sem);
> > > +		vma = find_vma(mm, start);
> > >  		if (!vma)
> > >  			return -ENOMEM;
> > >  		if (start < vma->vm_start) {
> > > @@ -811,7 +818,8 @@ static long madvise_dontneed_free(struct vm_area_struct *vma,
> > >   * Application wants to free up the pages and associated backing store.
> > >   * This is effectively punching a hole into the middle of a file.
> > >   */
> > > -static long madvise_remove(struct vm_area_struct *vma,
> > > +static long madvise_remove(struct mm_struct *mm,
> > > +				struct vm_area_struct *vma,
> > >  				struct vm_area_struct **prev,
> > >  				unsigned long start, unsigned long end)
> > >  {
> > > @@ -845,13 +853,13 @@ static long madvise_remove(struct vm_area_struct *vma,
> > >  	get_file(f);
> > >  	if (userfaultfd_remove(vma, start, end)) {
> > >  		/* mmap_sem was not released by userfaultfd_remove() */
> > > -		up_read(&current->mm->mmap_sem);
> > > +		up_read(&mm->mmap_sem);
> > >  	}
> > >  	error = vfs_fallocate(f,
> > >  				FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> > >  				offset, end - start);
> > >  	fput(f);
> > > -	down_read(&current->mm->mmap_sem);
> > > +	down_read(&mm->mmap_sem);
> > >  	return error;
> > >  }
> > >  
> > > @@ -925,21 +933,23 @@ static int madvise_inject_error(int behavior,
> > >  #endif
> > >  
> > >  static long
> > > -madvise_vma(struct vm_area_struct *vma, struct vm_area_struct **prev,
> > > +madvise_vma(struct task_struct *task, struct mm_struct *mm,
> > > +		struct vm_area_struct *vma, struct vm_area_struct **prev,
> > >  		unsigned long start, unsigned long end, int behavior)
> > >  {
> > >  	switch (behavior) {
> > >  	case MADV_REMOVE:
> > > -		return madvise_remove(vma, prev, start, end);
> > > +		return madvise_remove(mm, vma, prev, start, end);
> > >  	case MADV_WILLNEED:
> > >  		return madvise_willneed(vma, prev, start, end);
> > >  	case MADV_COLD:
> > > -		return madvise_cold(vma, prev, start, end);
> > > +		return madvise_cold(task, vma, prev, start, end);
> > >  	case MADV_PAGEOUT:
> > > -		return madvise_pageout(vma, prev, start, end);
> > > +		return madvise_pageout(task, vma, prev, start, end);
> > >  	case MADV_FREE:
> > >  	case MADV_DONTNEED:
> > > -		return madvise_dontneed_free(vma, prev, start, end, behavior);
> > > +		return madvise_dontneed_free(mm, vma, prev, start,
> > > +						end, behavior);
> > >  	default:
> > >  		return madvise_behavior(vma, prev, start, end, behavior);
> > >  	}
> > > @@ -984,67 +994,19 @@ madvise_behavior_valid(int behavior)
> > >  }
> > >  
> > >  /*
> > > - * The madvise(2) system call.
> > > + * madvise_common - request behavior hint to address range of the target process
> > >   *
> > > - * Applications can use madvise() to advise the kernel how it should
> > > - * handle paging I/O in this VM area.  The idea is to help the kernel
> > > - * use appropriate read-ahead and caching techniques.  The information
> > > - * provided is advisory only, and can be safely disregarded by the
> > > - * kernel without affecting the correct operation of the application.
> > > + * @task: task_struct got behavior hint, not giving the hint
> > > + * @mm: mm_struct got behavior hint, not giving the hint
> > > + * @start: base address of the hinted range
> > > + * @len_in: length of the hinted range
> > > + * @behavior: requested hint
> > >   *
> > > - * behavior values:
> > > - *  MADV_NORMAL - the default behavior is to read clusters.  This
> > > - *		results in some read-ahead and read-behind.
> > > - *  MADV_RANDOM - the system should read the minimum amount of data
> > > - *		on any access, since it is unlikely that the appli-
> > > - *		cation will need more than what it asks for.
> > > - *  MADV_SEQUENTIAL - pages in the given range will probably be accessed
> > > - *		once, so they can be aggressively read ahead, and
> > > - *		can be freed soon after they are accessed.
> > > - *  MADV_WILLNEED - the application is notifying the system to read
> > > - *		some pages ahead.
> > > - *  MADV_DONTNEED - the application is finished with the given range,
> > > - *		so the kernel can free resources associated with it.
> > > - *  MADV_FREE - the application marks pages in the given range as lazy free,
> > > - *		where actual purges are postponed until memory pressure happens.
> > > - *  MADV_REMOVE - the application wants to free up the given range of
> > > - *		pages and associated backing store.
> > > - *  MADV_DONTFORK - omit this area from child's address space when forking:
> > > - *		typically, to avoid COWing pages pinned by get_user_pages().
> > > - *  MADV_DOFORK - cancel MADV_DONTFORK: no longer omit this area when forking.
> > > - *  MADV_WIPEONFORK - present the child process with zero-filled memory in this
> > > - *              range after a fork.
> > > - *  MADV_KEEPONFORK - undo the effect of MADV_WIPEONFORK
> > > - *  MADV_HWPOISON - trigger memory error handler as if the given memory range
> > > - *		were corrupted by unrecoverable hardware memory failure.
> > > - *  MADV_SOFT_OFFLINE - try to soft-offline the given range of memory.
> > > - *  MADV_MERGEABLE - the application recommends that KSM try to merge pages in
> > > - *		this area with pages of identical content from other such areas.
> > > - *  MADV_UNMERGEABLE- cancel MADV_MERGEABLE: no longer merge pages with others.
> > > - *  MADV_HUGEPAGE - the application wants to back the given range by transparent
> > > - *		huge pages in the future. Existing pages might be coalesced and
> > > - *		new pages might be allocated as THP.
> > > - *  MADV_NOHUGEPAGE - mark the given range as not worth being backed by
> > > - *		transparent huge pages so the existing pages will not be
> > > - *		coalesced into THP and new pages will not be allocated as THP.
> > > - *  MADV_DONTDUMP - the application wants to prevent pages in the given range
> > > - *		from being included in its core dump.
> > > - *  MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump.
> > > - *
> > > - * return values:
> > > - *  zero    - success
> > > - *  -EINVAL - start + len < 0, start is not page-aligned,
> > > - *		"behavior" is not a valid value, or application
> > > - *		is attempting to release locked or shared pages,
> > > - *		or the specified address range includes file, Huge TLB,
> > > - *		MAP_SHARED or VMPFNMAP range.
> > > - *  -ENOMEM - addresses in the specified range are not currently
> > > - *		mapped, or are outside the AS of the process.
> > > - *  -EIO    - an I/O error occurred while paging in data.
> > > - *  -EBADF  - map exists, but area maps something that isn't a file.
> > > - *  -EAGAIN - a kernel resource was temporarily unavailable.
> > > + * @task could be a zombie leader if it calls sys_exit so accessing mm_struct
> > > + * via task->mm is prohibited. Please use @mm instead of task->mm.
> > >   */
> > > -SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> > > +static int madvise_common(struct task_struct *task, struct mm_struct *mm,
> > > +			unsigned long start, size_t len_in, int behavior)
> > >  {
> > >  	unsigned long end, tmp;
> > >  	struct vm_area_struct *vma, *prev;
> > > @@ -1082,10 +1044,10 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> > >  
> > >  	write = madvise_need_mmap_write(behavior);
> > >  	if (write) {
> > > -		if (down_write_killable(&current->mm->mmap_sem))
> > > +		if (down_write_killable(&mm->mmap_sem))
> > >  			return -EINTR;
> > >  	} else {
> > > -		down_read(&current->mm->mmap_sem);
> > > +		down_read(&mm->mmap_sem);
> > >  	}
> > >  
> > >  	/*
> > > @@ -1093,7 +1055,7 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> > >  	 * ranges, just ignore them, but return -ENOMEM at the end.
> > >  	 * - different from the way of handling in mlock etc.
> > >  	 */
> > > -	vma = find_vma_prev(current->mm, start, &prev);
> > > +	vma = find_vma_prev(mm, start, &prev);
> > >  	if (vma && start > vma->vm_start)
> > >  		prev = vma;
> > >  
> > > @@ -1118,7 +1080,7 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> > >  			tmp = end;
> > >  
> > >  		/* Here vma->vm_start <= start < tmp <= (end|vma->vm_end). */
> > > -		error = madvise_vma(vma, &prev, start, tmp, behavior);
> > > +		error = madvise_vma(task, mm, vma, &prev, start, tmp, behavior);
> > >  		if (error)
> > >  			goto out;
> > >  		start = tmp;
> > > @@ -1130,14 +1092,80 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> > >  		if (prev)
> > >  			vma = prev->vm_next;
> > >  		else	/* madvise_remove dropped mmap_sem */
> > > -			vma = find_vma(current->mm, start);
> > > +			vma = find_vma(mm, start);
> > >  	}
> > >  out:
> > >  	blk_finish_plug(&plug);
> > >  	if (write)
> > > -		up_write(&current->mm->mmap_sem);
> > > +		up_write(&mm->mmap_sem);
> > >  	else
> > > -		up_read(&current->mm->mmap_sem);
> > > +		up_read(&mm->mmap_sem);
> > >  
> > >  	return error;
> > >  }
> > > +
> > > +/*
> > > + * The madvise(2) system call.
> > > + *
> > > + * Applications can use madvise() to advise the kernel how it should
> > > + * handle paging I/O in this VM area.  The idea is to help the kernel
> > > + * use appropriate read-ahead and caching techniques.  The information
> > > + * provided is advisory only, and can be safely disregarded by the
> > > + * kernel without affecting the correct operation of the application.
> > > + *
> > > + * behavior values:
> > > + *  MADV_NORMAL - the default behavior is to read clusters.  This
> > > + *		results in some read-ahead and read-behind.
> > > + *  MADV_RANDOM - the system should read the minimum amount of data
> > > + *		on any access, since it is unlikely that the appli-
> > > + *		cation will need more than what it asks for.
> > > + *  MADV_SEQUENTIAL - pages in the given range will probably be accessed
> > > + *		once, so they can be aggressively read ahead, and
> > > + *		can be freed soon after they are accessed.
> > > + *  MADV_WILLNEED - the application is notifying the system to read
> > > + *		some pages ahead.
> > > + *  MADV_DONTNEED - the application is finished with the given range,
> > > + *		so the kernel can free resources associated with it.
> > > + *  MADV_FREE - the application marks pages in the given range as lazy free,
> > > + *		where actual purges are postponed until memory pressure happens.
> > > + *  MADV_REMOVE - the application wants to free up the given range of
> > > + *		pages and associated backing store.
> > > + *  MADV_DONTFORK - omit this area from child's address space when forking:
> > > + *		typically, to avoid COWing pages pinned by get_user_pages().
> > > + *  MADV_DOFORK - cancel MADV_DONTFORK: no longer omit this area when forking.
> > > + *  MADV_WIPEONFORK - present the child process with zero-filled memory in this
> > > + *              range after a fork.
> > > + *  MADV_KEEPONFORK - undo the effect of MADV_WIPEONFORK
> > > + *  MADV_HWPOISON - trigger memory error handler as if the given memory range
> > > + *		were corrupted by unrecoverable hardware memory failure.
> > > + *  MADV_SOFT_OFFLINE - try to soft-offline the given range of memory.
> > > + *  MADV_MERGEABLE - the application recommends that KSM try to merge pages in
> > > + *		this area with pages of identical content from other such areas.
> > > + *  MADV_UNMERGEABLE- cancel MADV_MERGEABLE: no longer merge pages with others.
> > > + *  MADV_HUGEPAGE - the application wants to back the given range by transparent
> > > + *		huge pages in the future. Existing pages might be coalesced and
> > > + *		new pages might be allocated as THP.
> > > + *  MADV_NOHUGEPAGE - mark the given range as not worth being backed by
> > > + *		transparent huge pages so the existing pages will not be
> > > + *		coalesced into THP and new pages will not be allocated as THP.
> > > + *  MADV_DONTDUMP - the application wants to prevent pages in the given range
> > > + *		from being included in its core dump.
> > > + *  MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump.
> > 
> > Just a trivial suggestion.  How about adding brief descriptions for the
> > 'MADV_COLD' and 'MADV_PAGEOUT' here, probably with another patch?
> 
> How about this?
> Feel free to suggest better wording.
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 3aa9aec6bfd9..78b3ab789486 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1169,6 +1169,10 @@ static int madvise_common(struct task_struct *task, struct mm_struct *mm,
>   *  MADV_DONTDUMP - the application wants to prevent pages in the given range
>   *		from being included in its core dump.
>   *  MADV_DODUMP - cancel MADV_DONTDUMP: no longer exclude from core dump.
> + *  MADV_COLD - the application uses the memory less so the kernel can deactivate
> + *  		the memory to evict them quickly when the memory pressure happen.
> + *  MADV_PAGEOUT - the application uses the memroy very rarely so kernel can
> + *  		page out the memory instantly.

Looks good to me :)


Thanks,
SeongJae Park

>   *
>   * return values:
>   *  zero    - success
> 
> Thanks.
> 

^ permalink raw reply

* Re: [PATCH 2/4] mm: introduce external memory hinting API
From: Minchan Kim @ 2020-01-13 18:39 UTC (permalink / raw)
  To: Kirill Tkhai
  Cc: Andrew Morton, LKML, linux-mm, linux-api, oleksandr,
	Suren Baghdasaryan, Tim Murray, Daniel Colascione, Sandeep Patil,
	Sonny Rao, Brian Geffon, Michal Hocko, Johannes Weiner,
	Shakeel Butt, John Dias, christian.brauner
In-Reply-To: <56ea0927-ad2e-3fbd-3366-3813330f6cec@virtuozzo.com>

On Mon, Jan 13, 2020 at 11:47:11AM +0300, Kirill Tkhai wrote:
> On 11.01.2020 00:34, Minchan Kim wrote:
> > There are usecases that System Management Software(SMS) want to give
> > a memory hint to other processes because it's not known to the
> > application. In the case of Android, ActivityManagerService daemon
> > manges app's life cycle and that daemon must be able to initiate
> > reclaim on its own without any app involvement.
> > 
> > To solve the issue, this patch introduces new syscall process_madvise(2).
> > It uses pidfd of an external processs to give the hint.
> > 
> >  int process_madvise(int pidfd, void *addr, size_t length, int advise,
> > 			unsigned long flag);
> > 
> > Since it could affect other process's address range, only privileged
> > process(CAP_SYS_PTRACE) or something else(e.g., being the same UID)
> > gives it the right to ptrace the process could use it successfully.
> > The flag argument is reserved for future use if we need to extend the
> > API.
> > 
> > Supporting all hints madvise has/will supported/support to process_madvise
> > is rather risky. Because we are not sure all hints make sense from external
> > process and implementation for the hint may rely on the caller being
> > in the current context so it could be error-prone. Thus, I just limited
> > hints as MADV_[COLD|PAGEOUT] in this patch.
> > 
> > If someone want to add other hints, we could hear hear the usecase and
> > review it for each hint. It's more safe for maintainace rather than
> > introducing a buggy syscall but hard to fix it later.
> > 
> > Signed-off-by: Minchan Kim <minchan@kernel.org>
> > ---
> >  arch/alpha/kernel/syscalls/syscall.tbl      |  1 +
> >  arch/arm/tools/syscall.tbl                  |  1 +
> >  arch/arm64/include/asm/unistd.h             |  2 +-
> >  arch/arm64/include/asm/unistd32.h           |  2 +
> >  arch/ia64/kernel/syscalls/syscall.tbl       |  1 +
> >  arch/m68k/kernel/syscalls/syscall.tbl       |  1 +
> >  arch/microblaze/kernel/syscalls/syscall.tbl |  1 +
> >  arch/mips/kernel/syscalls/syscall_n32.tbl   |  1 +
> >  arch/mips/kernel/syscalls/syscall_n64.tbl   |  1 +
> >  arch/parisc/kernel/syscalls/syscall.tbl     |  1 +
> >  arch/powerpc/kernel/syscalls/syscall.tbl    |  1 +
> >  arch/s390/kernel/syscalls/syscall.tbl       |  1 +
> >  arch/sh/kernel/syscalls/syscall.tbl         |  1 +
> >  arch/sparc/kernel/syscalls/syscall.tbl      |  1 +
> >  arch/x86/entry/syscalls/syscall_32.tbl      |  1 +
> >  arch/x86/entry/syscalls/syscall_64.tbl      |  1 +
> >  arch/xtensa/kernel/syscalls/syscall.tbl     |  1 +
> >  include/linux/syscalls.h                    |  2 +
> >  include/uapi/asm-generic/unistd.h           |  5 +-
> >  kernel/sys_ni.c                             |  1 +
> >  mm/madvise.c                                | 64 +++++++++++++++++++++
> >  21 files changed, 89 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
> > index e56950f23b49..776c61803315 100644
> > --- a/arch/alpha/kernel/syscalls/syscall.tbl
> > +++ b/arch/alpha/kernel/syscalls/syscall.tbl
> > @@ -477,3 +477,4 @@
> >  # 545 reserved for clone3
> >  546	common	watch_devices			sys_watch_devices
> >  547	common	openat2				sys_openat2
> > +548	common	process_madvise			sys_process_madvise
> > diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
> > index 7fb2f4d59210..a43381542276 100644
> > --- a/arch/arm/tools/syscall.tbl
> > +++ b/arch/arm/tools/syscall.tbl
> > @@ -451,3 +451,4 @@
> >  435	common	clone3				sys_clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +438	common	process_madvise			sys_process_madvise
> > diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
> > index 8aa00ccb0b96..b722e47377a5 100644
> > --- a/arch/arm64/include/asm/unistd.h
> > +++ b/arch/arm64/include/asm/unistd.h
> > @@ -38,7 +38,7 @@
> >  #define __ARM_NR_compat_set_tls		(__ARM_NR_COMPAT_BASE + 5)
> >  #define __ARM_NR_COMPAT_END		(__ARM_NR_COMPAT_BASE + 0x800)
> >  
> > -#define __NR_compat_syscalls		438
> > +#define __NR_compat_syscalls		439
> >  #endif
> >  
> >  #define __ARCH_WANT_SYS_CLONE
> > diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
> > index 31f0ce25719e..5c82557d408f 100644
> > --- a/arch/arm64/include/asm/unistd32.h
> > +++ b/arch/arm64/include/asm/unistd32.h
> > @@ -883,6 +883,8 @@ __SYSCALL(__NR_clone3, sys_clone3)
> >  __SYSCALL(__NR_watch_devices, sys_watch_devices)
> >  #define __NR_openat2 437
> >  __SYSCALL(__NR_openat2, sys_openat2)
> > +#define __NR_openat2 438
> > +__SYSCALL(__NR_process_madvise, process_madvise)
> >  
> >  /*
> >   * Please add new compat syscalls above this comment and update
> > diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl
> > index b9aa59931905..c156abc9a298 100644
> > --- a/arch/ia64/kernel/syscalls/syscall.tbl
> > +++ b/arch/ia64/kernel/syscalls/syscall.tbl
> > @@ -358,3 +358,4 @@
> >  # 435 reserved for clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +438	common	process_madvise			sys_process_madvise
> > diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
> > index 868c1ef89d35..5b6034b6650f 100644
> > --- a/arch/m68k/kernel/syscalls/syscall.tbl
> > +++ b/arch/m68k/kernel/syscalls/syscall.tbl
> > @@ -437,3 +437,4 @@
> >  # 435 reserved for clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +438	common	process_madvise			sys_process_madvise
> > diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
> > index 544b4cef18b3..4bef584af09c 100644
> > --- a/arch/microblaze/kernel/syscalls/syscall.tbl
> > +++ b/arch/microblaze/kernel/syscalls/syscall.tbl
> > @@ -443,3 +443,4 @@
> >  435	common	clone3				sys_clone3
> >  436	common	watch_devices			sys_watch_devices
> >  437	common	openat2				sys_openat2
> > +438	common	process_madvise			sys_process_madvise
> > diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
> > index 05e8aee5dae7..94fbd0fcccce 100644
> > --- a/arch/mips/kernel/syscalls/syscall_n32.tbl
> > +++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
> > @@ -376,3 +376,4 @@
> >  435	n32	clone3				__sys_clone3
> >  436	n32	watch_devices			sys_watch_devices
> >  437	n32	openat2				sys_openat2
> > +437	n32	process_madivse			sys_process_madvise
> 
> 438. And several places below has the same mistake.

Thanks. I fixed.

< snip >

> > diff --git a/mm/madvise.c b/mm/madvise.c
> > index 0c901de531e4..e15dfb4df7bf 100644
> > --- a/mm/madvise.c
> > +++ b/mm/madvise.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/falloc.h>
> >  #include <linux/fadvise.h>
> >  #include <linux/sched.h>
> > +#include <linux/sched/mm.h>
> >  #include <linux/ksm.h>
> >  #include <linux/fs.h>
> >  #include <linux/file.h>
> > @@ -993,6 +994,18 @@ madvise_behavior_valid(int behavior)
> >  	}
> >  }
> >  
> > +static bool
> > +process_madvise_behavior_valid(int behavior)
> > +{
> > +	switch (behavior) {
> > +	case MADV_COLD:
> > +	case MADV_PAGEOUT:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> >  /*
> >   * madvise_common - request behavior hint to address range of the target process
> >   *
> > @@ -1169,3 +1182,54 @@ SYSCALL_DEFINE3(madvise, unsigned long, start, size_t, len_in, int, behavior)
> >  {
> >  	return madvise_common(current, current->mm, start, len_in, behavior);
> >  }
> > +
> > +SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
> > +		size_t, len_in, int, behavior, unsigned long, flags)
> 
> I don't like the interface. The fact we have pidfd does not mean,
> we have to use it for new syscalls always. A user may want to set
> madvise for specific pid from console and pass pid as argument.
> pidfd would be an overkill in this case.

I am curious what is our plan for pid and pidfd once we introduced pidfd.
Ccing pidfd maintainer.

Since pid has clear problems, pidfd was born so not sure we should keep
both options for new coming API.

Since this is hinting API, performance wouldn't be critical requirment.
Rather than, it aims for a certain target process so that keep the process
during the operation without concern of reusing pid, which is perfectly
matched with pidfd.

> We usually call "kill -9 pid" from console. Why shouldn't process_madvise()
> allow this?

It's a syscall but not command which uses the syscall for implementation.

> 
> I suggent to extend first argument to work with both pid and pidfd.
> Look at what we have for waitid(idtype, id_t id, ...) for example:
> 
>        idtype == P_PID
>               Wait for the child whose process ID matches id.
> 
>        idtype == P_PIDFD (since Linux 5.4)
>               Wait for the child referred to by the PID file descriptor specified in id.  (See pidfd_open(2) for  further  information  on
>               PID file descriptors.)

IMO, it's okay to extend existing syscall to support new pidfd but I'm not
convinced why we should support both options for this memory hinting API
fron the beginning.

I'm not strong against of supporting both options but it would be much better
to have strong justification or guide whehter we should support both or only
pidfd so that upcoming APIs will follow the strategy.

> 
> We may use @flags argument for this.

> 
> > +{
> > +	int ret;
> > +	struct fd f;
> > +	struct pid *pid;
> > +	struct task_struct *task;
> > +	struct mm_struct *mm;
> > +
> > +	if (flags != 0)
> > +		return -EINVAL;
> > +
> > +	if (!process_madvise_behavior_valid(behavior))
> > +		return -EINVAL;
> > +
> > +	f = fdget(pidfd);
> > +	if (!f.file)
> > +		return -EBADF;
> > +
> > +	pid = pidfd_pid(f.file);
> > +	if (IS_ERR(pid)) {
> > +		ret = PTR_ERR(pid);
> > +		goto err;
> > +	}
> 
> From here:
> 
> > +	rcu_read_lock();
> > +	task = pid_task(pid, PIDTYPE_PID);
> > +	if (!task) {
> > +		rcu_read_unlock();
> > +		ret = -ESRCH;
> > +		goto err;
> > +	}
> > +
> > +	get_task_struct(task);
> > +	rcu_read_unlock();
> 
> to here your code is a copy of existing get_pid_task(). We should the primitive instead.

Fixed.

> 
> > +
> > +	mm = mm_access(task, PTRACE_MODE_ATTACH_FSCREDS);
> > +	if (IS_ERR_OR_NULL(mm)) {
> > +		ret = IS_ERR(mm) ? PTR_ERR(mm) : -ESRCH;
> > +		goto release_task;
> > +	}
> > +
> > +	ret = madvise_common(task, mm, start, len_in, behavior);
> > +	mmput(mm);
> > +release_task:
> > +	put_task_struct(task);
> > +err:
> 
> Maybe s/err:/fdput:/ for uniformity with the above "release_task:"?

Fixed.

Thanks for the review.

^ permalink raw reply

* Re: [PATCH 2/4] mm: introduce external memory hinting API
From: Minchan Kim @ 2020-01-13 18:44 UTC (permalink / raw)
  To: Christian Brauner
  Cc: Kirill Tkhai, Michal Hocko, Andrew Morton, LKML, linux-mm,
	linux-api-u79uwXL29TY76Z2rM5mHXA,
	oleksandr-H+wXaHxf7aLQT0dZR+AlfA, Suren Baghdasaryan, Tim Murray,
	Daniel Colascione, Sandeep Patil, Sonny Rao, Brian Geffon,
	Johannes Weiner, Shakeel Butt, John Dias
In-Reply-To: <20200113104256.5ujbplyec2sk4onn@wittgenstein>

On Mon, Jan 13, 2020 at 11:42:57AM +0100, Christian Brauner wrote:
> On Mon, Jan 13, 2020 at 11:47:11AM +0300, Kirill Tkhai wrote:

< snip >

> > > +SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
> > > +		size_t, len_in, int, behavior, unsigned long, flags)
> > 
> > I don't like the interface. The fact we have pidfd does not mean,
> > we have to use it for new syscalls always. A user may want to set
> > madvise for specific pid from console and pass pid as argument.
> > pidfd would be an overkill in this case.
> > We usually call "kill -9 pid" from console. Why shouldn't process_madvise()
> > allow this?
> > 
> > I suggent to extend first argument to work with both pid and pidfd.
> > Look at what we have for waitid(idtype, id_t id, ...) for example:
> > 
> >        idtype == P_PID
> >               Wait for the child whose process ID matches id.
> > 
> >        idtype == P_PIDFD (since Linux 5.4)
> >               Wait for the child referred to by the PID file descriptor specified in id.  (See pidfd_open(2) for  further  information  on
> >               PID file descriptors.)
> > 
> > We may use @flags argument for this.
> 
> Sorry for chiming in just a comment. Overall, I don't particularly care
> how or if you integrate pidfd here. One thing I would like to point out
> is that we're working on a patch to place new features under pidfd
> specific flags. This e.g. means a pidfd would be only be able to be used
> for madvise operations (or getfd operations) if it was created with that
> specific flag set making it easier to share them with other processes.
> So if you integrate them here I would be quite thankful if you target
> the patchset for the v5.7 merge window, not for v5.6.

Hi Christian,
Sorry but I couldn't understand your point.
Could you clarify what you meant?

Thanks.

^ permalink raw reply

* Re: [PATCH 2/4] mm: introduce external memory hinting API
From: Christian Brauner @ 2020-01-13 19:10 UTC (permalink / raw)
  To: Minchan Kim
  Cc: Kirill Tkhai, Michal Hocko, Andrew Morton, LKML, linux-mm,
	linux-api, oleksandr, Suren Baghdasaryan, Tim Murray,
	Daniel Colascione, Sandeep Patil, Sonny Rao, Brian Geffon,
	Johannes Weiner, Shakeel Butt, John Dias
In-Reply-To: <20200113184408.GD110363@google.com>

On Mon, Jan 13, 2020 at 10:44:08AM -0800, Minchan Kim wrote:
> On Mon, Jan 13, 2020 at 11:42:57AM +0100, Christian Brauner wrote:
> > On Mon, Jan 13, 2020 at 11:47:11AM +0300, Kirill Tkhai wrote:
> 
> < snip >
> 
> > > > +SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
> > > > +		size_t, len_in, int, behavior, unsigned long, flags)
> > > 
> > > I don't like the interface. The fact we have pidfd does not mean,
> > > we have to use it for new syscalls always. A user may want to set
> > > madvise for specific pid from console and pass pid as argument.
> > > pidfd would be an overkill in this case.
> > > We usually call "kill -9 pid" from console. Why shouldn't process_madvise()
> > > allow this?
> > > 
> > > I suggent to extend first argument to work with both pid and pidfd.
> > > Look at what we have for waitid(idtype, id_t id, ...) for example:
> > > 
> > >        idtype == P_PID
> > >               Wait for the child whose process ID matches id.
> > > 
> > >        idtype == P_PIDFD (since Linux 5.4)
> > >               Wait for the child referred to by the PID file descriptor specified in id.  (See pidfd_open(2) for  further  information  on
> > >               PID file descriptors.)
> > > 
> > > We may use @flags argument for this.
> > 
> > Sorry for chiming in just a comment. Overall, I don't particularly care
> > how or if you integrate pidfd here. One thing I would like to point out
> > is that we're working on a patch to place new features under pidfd
> > specific flags. This e.g. means a pidfd would be only be able to be used
> > for madvise operations (or getfd operations) if it was created with that
> > specific flag set making it easier to share them with other processes.
> > So if you integrate them here I would be quite thankful if you target
> > the patchset for the v5.7 merge window, not for v5.6.
> 
> Hi Christian,
> Sorry but I couldn't understand your point.
> Could you clarify what you meant?

Hi Minchan,

Sure. When you create a pidfd, e.g. with clone3() and you'd wanted to
use it for madvise you'd need to set a flag like pidfd_cap_madvise or
pidfd_feature_madvise when you create the pidfd. Only if the pidfd was
created with that flag set could you use it with madvise (This does not
affect the permission checking you're performing here.). This has come
up a couple of times and becomes more relevant now that people keep
adding new features on top of pidfd and is similar to what we are now
doing with openat2().

Christian

^ permalink raw reply

* Re: [PATCH 2/4] mm: introduce external memory hinting API
From: Daniel Colascione @ 2020-01-13 19:18 UTC (permalink / raw)
  To: Kirill Tkhai
  Cc: Minchan Kim, Andrew Morton, LKML, linux-mm, Linux API,
	oleksandr-H+wXaHxf7aLQT0dZR+AlfA, Suren Baghdasaryan, Tim Murray,
	Sandeep Patil, Sonny Rao, Brian Geffon, Michal Hocko,
	Johannes Weiner, Shakeel Butt, John Dias
In-Reply-To: <56ea0927-ad2e-3fbd-3366-3813330f6cec-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org>

On Mon, Jan 13, 2020, 12:47 AM Kirill Tkhai <ktkhai-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org> wrote:
> > +SYSCALL_DEFINE5(process_madvise, int, pidfd, unsigned long, start,
> > +             size_t, len_in, int, behavior, unsigned long, flags)
>
> I don't like the interface. The fact we have pidfd does not mean,
> we have to use it for new syscalls always. A user may want to set
> madvise for specific pid from console and pass pid as argument.
> pidfd would be an overkill in this case.
> We usually call "kill -9 pid" from console. Why shouldn't process_madvise()
> allow this?

All new APIs should use pidfds: they're better than numeric PIDs in
every way. If a program wants to allow users to specify processes by
numeric PID, it can parse that numeric PID, open the corresponding
pidfd, and then use that pidfd with whatever system call it wants.
It's not necessary to support numeric PIDs at the system call level to
allow a console program to identify a process by numeric PID.

^ 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