Re: [PATCH] mm/fadvise: validate offset in generic_fadvise

[Kevin Lourenco <klourencodev@gmail.com>]

Hi David,

Thank you for your thoughts and recommendations. Well noted, I will
now start limiting the commit message to those specifications :)

For point (1):

I see POSIX definition like this: "The byte position in the file where
the next I/O operation begins."
(https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html?utm_source=chatgpt.com),
so what is almost certain is that a negative offset makes no sense /
there is no hidden mechanism or anything. I don't think we can
accidentally break users, my intuition is that nobody ever
intentionally passes a negative offset (probably the same for len).

We could rely exclusively on the user to ensure the offset is positive
and follow POSIX recommendation. But since errors can occur, I think
it can be hard to debug where a simple test, the same for len, could
have made their experience easier. I think this is also the path
FreeBSD chose: https://github.com/freebsd/freebsd-src/blob/main/sys/kern/vfs_syscalls.c#L4918

For point (2):

Since I think nobody intentionally passes negative values and since
that makes no sense semantically speaking, I think we can consider it
unnecessary to update the man page right now and only think about it
in the future if a negative offset gains a new meaning, for example.

Wdyt?

Le mar. 23 déc. 2025 à 10:45, David Hildenbrand (Red Hat)
<david@kernel.org> a écrit :
>
> On 12/22/25 15:18, klourencodev@gmail.com wrote:
> > From: Kevin Lourenco <klourencodev@gmail.com>
> >
> > When converted to (u64) for page calculations, a negative offset
> > can produce extremely large page indices. This may lead to issues in certain advice modes (excessive readahead or
> > cache invalidation)
> >
> > offsets are normally non-negative, but the API does not guarantee this. Since 'len' is already
> > validated, checking 'offset' here is reasonable to prevent potential system instability.
>
> Hi,
>
> we tend to break lines as 72 chars in the patch description. I assume
> Andrew will fix that up or already did it :)
>
> >
> > Signed-off-by: Kevin Lourenco <k.lourenco@criteo.com>
> > ---
> >   mm/fadvise.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/fadvise.c b/mm/fadvise.c
> > index 67028e30aa91..b63fe21416ff 100644
> > --- a/mm/fadvise.c
> > +++ b/mm/fadvise.c
> > @@ -43,7 +43,7 @@ int generic_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
> >               return -ESPIPE;
> >
> >       mapping = file->f_mapping;
> > -     if (!mapping || len < 0)
> > +     if (!mapping || len < 0 || offset < 0)
> >               return -EINVAL;
> >
> >       bdi = inode_to_bdi(mapping->host);
>
> The man page of fadvise64()/posix_madvise() is a bit unclear. It doesn't
> really specify what's supposed to happen on negative size or offset.
>
> Staring at test cases in LTP, we seem to have:
>
> * Check the value that posix_fadvise returns for wrong ADVISE value
> * Check the value that posix_fadvise returns for wrong file descriptor
> * Check the value that posix_fadvise returns for wrong ADVISE value
> * Check the value that posix_fadvise returns for pipe descriptor
>
> And we primarily only seem to test what's documented in the man page to
> fail.
>
> Which raises the questions:
> (1) Could we accidentally break some users out there?
> (2) Should we update the man page to document what is supposed to happen
>      with negative size or offset.
>
> --
> Cheers
>
> David