linux-kernel - Re: [PATCH v8 2/3] cachestat: implement cachestat syscall

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <12e9afc8-cec4-4fce-ad81-09790cbe3938@app.fastmail.com>
Date:   Fri, 27 Jan 2023 21:29:47 +0100
From:   "Arnd Bergmann" <arnd@...db.de>
To:     "Johannes Weiner" <hannes@...xchg.org>
Cc:     "Nhat Pham" <nphamcs@...il.com>,
        "Andrew Morton" <akpm@...ux-foundation.org>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, bfoster@...hat.com,
        "Matthew Wilcox" <willy@...radead.org>, linux-api@...r.kernel.org,
        kernel-team@...a.com
Subject: Re: [PATCH v8 2/3] cachestat: implement cachestat syscall

On Fri, Jan 27, 2023, at 20:46, Johannes Weiner wrote:
> On Fri, Jan 27, 2023 at 04:46:38PM +0100, Arnd Bergmann wrote:
>> 
>> - if you make it a 32-bit type, this breaks calling it from
>>   normal userspace that defines off_t as a 64-bit type
>> 
>> - if you change it to a 64-bit loff_t, there are three
>>   separate calling conventions for 64-bit, 32-bit with
>>   aligned register pairs and other 32-bit, plus you
>>   exceed the usual limit of six system call arguments
>
> That's a good point, thanks for raising it, Arnd.
>
>> A separate problem may be the cstat_version argument, usually
>> we don't use interface versions but instead use a new
>> system call number if something changes in an incompatible
>> way.
>
> I suppose from a userspace POV, a version argument vs calling a
> separate syscall doesn't make much of a difference. So going with
> loff_t and dropping cstat_version seems like a sensible way forward.
>
> As an added bonus, versioning the syscall itself means the signature
> can change in v2. This allows dropping the unused flags arg for now.
>
> That would leave it at:
>
>   int cachestat(unsigned int, loff_t, size_t len, struct cachestat *cstat);

There is still a problem of incompatible calling conventions:
on architectures that require even/odd register pairs, this would
end up like

int cachestat(unsigned int, long unused, u32 off_low, u32 off_high,
              size_t len, struct cachestat *cstat);

A more portable way to do this would be to pass the offset by
reference, but that makes it a bit awkward in userspace.

Or the arguments could be rearranged to put the low/high argument
pair first/second, third/fourth or fifth/sixth argument, at least
on the kernel ABI to avoid having another situation like
sys_arm_fadvise64_64.

> and should we ever require extensions - new fields, flags - they would
> come through a new cachestat2().
>
> Would anybody have objections to that?

If there is room for another argument, I would keep the 'flags'
as a way for extending in a compatible way, that is pretty standard
now, just not flags plus version.

      Arnd