Message-ID: <CAKEwX=Ppf=WbOuV2Rh3+V8ohOYXo=CnfSu9qqSh-DpVvfy2nhA@mail.gmail.com>
Date:   Wed, 11 Jan 2023 10:00:14 -0800
From:   Nhat Pham <nphamcs@...il.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     hannes@...xchg.org, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, bfoster@...hat.com,
        willy@...radead.org, kernel-team@...a.com
Subject: Re: [PATCH v4 3/4] cachestat: implement cachestat syscall

On Wed, Dec 21, 2022 at 4:37 PM Nhat Pham <nphamcs@...il.com> wrote:
>
> On Fri, Dec 16, 2022 at 1:48 PM Andrew Morton <akpm@...ux-foundation.org> wrote:
> >
> > On Fri, 16 Dec 2022 11:21:48 -0800 Nhat Pham <nphamcs@...il.com> wrote:
> >
> > > Implement a new syscall that queries cache state of a file and
> > > summarizes the number of cached pages, number of dirty pages, number of
> > > pages marked for writeback, number of (recently) evicted pages, etc. in
> > > a given range.
> > >
> > > NAME
> > >     cachestat - query the page cache status of a file.
> > >
> > > SYNOPSIS
> > >     #include <sys/mman.h>
> > >
> > >     struct cachestat {
> > >         __u64 nr_cache;
> > >         __u64 nr_dirty;
> > >         __u64 nr_writeback;
> > >         __u64 nr_evicted;
> > >         __u64 nr_recently_evicted;
> > >     };
> > >
> > >     int cachestat(unsigned int fd, off_t off, size_t len,
> > >           size_t cstat_size, struct cachestat *cstat,
> > >           unsigned int flags);
> > >
> > > DESCRIPTION
> > >     cachestat() queries the number of cached pages, number of dirty
> > >     pages, number of pages marked for writeback, number of (recently)
> > >     evicted pages, in the bytes range given by `off` and `len`.
> >
> > I suggest this be spelled out better: "number of evicted and number or
> > recently evicted pages".
> >
> > I suggest this clearly tell readers what an "evicted" page is - they
> > aren't kernel programmers!
>
> Valid points - I'll try to explain this more clearly in the future
> versions of this patch series, especially in the draft man page.
>
> >
> > What is the benefit of the "recently evicted" pages?  "recently" seems
> > very vague - what use is this to anyone?
>
> This eviction recency semantics comes from the LRU's refault
> computation. Users of cachestat might be interested in two very
> different questions:
>
> 1. How many pages are not resident in the page cache.
> 2. How many pages are recently evicted (recently enough that
>     their refault will be construed as memory pressure).
>
> The first question is answered with nr_evicted, whereas the second
> is answered with nr_recently_evicted.
>
> I will figure out a way to explain this better in the next version. Users
> definitely do not need to know the nitty gritty details of LRU logic,
> but they should know the general idea of each field at least.
>
> >
> > >     These values are returned in a cachestat struct, whose address is
> > >     given by the `cstat` argument.
> > >
> > >     The `off` and `len` arguments must be non-negative integers. If
> > >     `len` > 0, the queried range is [`off`, `off` + `len`]. If `len` ==
> > >     0, we will query in the range from `off` to the end of the file.
> > >
> > >     `cstat_size` allows users to obtain partial results. The syscall
> > >     will copy the first `csstat_size` bytes to the specified userspace
> > >     memory. `cstat_size` must be a non-negative value that is no larger
> > >     than the current size of the cachestat struct.
> > >
> > >     The `flags` argument is unused for now, but is included for future
> > >     extensibility. User should pass 0 (i.e no flag specified).
> >
> > Why is `flags' here?  We could add an unused flags arg to any syscall,
> > but we don't.  What's the plan?
>
> I included this field to ensure that cachestat can be extended safely,
> especially when different users might want different things out of it.
>
> For instance, in the future there might be new fields/computations
> that are too heavy for certain use cases - a flag could be used to
> disable/skip such fields/computations.
>
> Another thing it might be used for is the huge page counting -
> we have not implemented this in this version yet, but it might
> introduce murky semantics to new/existing fields in struct
> cachestat. Or maybe not - but worst case scenario we can
> leave this decision to the users to decide through flags.
>
> I'm sure there are more potential pitfalls that the flags could
> save us from, but these are the two on top of my head.
>
> >
> > Are there security implications?  If I know that some process has a
> > file open, I can use cachestat() to infer which parts of that file
> > they're looking at (like mincore(), I guess).  And I can infer which
> > parts they're writing to, unlike mincore().
>
> This one, I'm not 100% sure, but it is a valid concern. Let me think
> about it and discuss with more security-oriented minds before
> responding to this.

Hmm I've given it some more thought. The syscall does not seem to
expose any extra security issue, given that the user already has
read permission to the file itself (since they have an fd to that file).
This means that the user can already know the underlying data in its
entirety, which seems like much more information (and as a result,
security risk) than the cache status itself.

Do you have something concrete in mind that I might have missed?

>
> >
> > I suggest the [patch 1/4] fixup be separated from this series.
>
> Sounds good! I'll loop Johannes in about this breakup as well.

Powered by blists - more mailing lists