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

["David Hildenbrand (Red Hat)" <david@kernel.org>]

On 12/23/25 17:16, Kevin Lourenco wrote:
> Hi David,

Hi!

> 
> 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.

All I find in that regard is

"... starting at offset and continuing for len bytes. The specified 
range need not currently exist in the file. If len is zero, all data 
from offset to the largest possible value of the file offset for that 
file shall be specified. "

So, something with a negative offset sure does not exist in the file.

Which makes me wonder whether we are here in a scenario where we would 
simply ignore everything "negative" (clamp offset to 0), simply return 0 
(because it's not documented to create an error), or whether we are in 
undefined behavior land and can simply return EINVAL.

> 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).

Some programs do stupid things, you would be surprised :)

> 
> 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

That's very good thing to note!

Interestingly, they also check

	offset > OFF_MAX - len

So checking that offset+len will not overflow. Do we want something 
similar? (check_add_overflow() etc. )

> 
> 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

Well, then why document negative length? ;) We should be as clear as 
possible in the documentation.

If POSIX would have been clearer we wouldn't have this discussion :)

> 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?

Likely we should really update the man page to reflect reality. 
"Starting with Linux v7.0, posix_fadvise() will fail with ...".

Given that FreeBSD rejects negative offsets I guess we are good.

-- 
Cheers

David