MIME-Version: 1.0
References: <20230203190413.2559707-1-nphamcs@gmail.com> <20230203190413.2559707-3-nphamcs@gmail.com>
 <834254e6-a9c2-440c-9c67-fc5fa5ca43bc@app.fastmail.com>
In-Reply-To: <834254e6-a9c2-440c-9c67-fc5fa5ca43bc@app.fastmail.com>
From:   Nhat Pham <nphamcs@gmail.com>
Date:   Sat, 11 Feb 2023 23:17:34 -0800
Message-ID: <CAKEwX=OCURhs77Rdz81m8xyOKER_FvmVGVAKdmSWMgG--jWkKA@mail.gmail.com>
Subject: Re: [PATCH v9 2/3] cachestat: implement cachestat syscall
To:     Arnd Bergmann <arnd@arndb.de>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Johannes Weiner <hannes@cmpxchg.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, bfoster@redhat.com,
        Matthew Wilcox <willy@infradead.org>,
        linux-api@vger.kernel.org, kernel-team@meta.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sun, Feb 5, 2023 at 1:56 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Fri, Feb 3, 2023, at 20:04, Nhat Pham wrote:
>
> > +SYSCALL_DEFINE5(cachestat, unsigned int, fd, loff_t, off, size_t, len,
> > +             struct cachestat __user *, cstat, unsigned int, flags)
> > +{
> > +     return ksys_cachestat(fd, off, len, cstat, flags);
> > +}
> > +
> > +#ifdef CONFIG_COMPAT
> > +COMPAT_SYSCALL_DEFINE6(cachestat, unsigned int, fd,
> > compat_arg_u64_dual(off),
> > +             size_t, len, struct cachestat __user *, cstat, unsigned int, flags)
> > +{
> > +     return ksys_cachestat(fd, compat_arg_u64_glue(off), len, cstat,
> > flags);
> > +}
>
> This still looks wrong to me, as this compat definition does not match
> the native variant on architectures that require 64-bit arguments to
> be passed in aligned register pairs, such as arm, mips or ppc, but
> not x86, s390 or riscv.

(looks like I sent the last email in the wrong format - resending this
in plain text to see if the problem persists...)

Oh I see - thanks for pointing that out! And the last bit means this
is a non-issue for x86, s390 or riscv right?

And iirc from the last thread, this is fixable via a simple reordering
of the args in order to properly align the 64-bit arguments, for e.g:

SYSCALL_DEFINE5(cachestat, loff_t, off, unsigned int, fd, size_t, len,
              struct cachestat __user *, cstat, unsigned int, flags)

...

COMPAT_SYSCALL_DEFINE6(cachestat, compat_arg_u64_dual(off),
             unsigned int, fd, size_t, len, struct cachestat __user *, cstat,
             unsigned int, flags)


It looks a bit odd to me that fd is not the first argument, but perhaps this
is an acceptable sacrifice to avoid unused arg and keep the flags...

Let me know what you think about this!

>
>      Arnd