Re: [PATCH] fadvise: avoid EINVAL if user input is valid

[Hillf Danton]

On Sat, Feb 25, 2012 at 10:27 AM, Eric Wong <normalperson@xxxxxxxx> wrote:
> The kernel is not required to act on fadvise, so fail silently
> and ignore advice as long as it has a valid descriptor and
> parameters.
>
> Cc: linux-mm@xxxxxxxxx
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Eric Wong <normalperson@xxxxxxxx>
> ---
>
> ÂOf course I wouldn't knowingly call posix_fadvise() on a file in
> Âtmpfs, but a userspace app often doesn't know (nor should it
> Âcare) what type of filesystem it's on.
>
> ÂI encountered EINVAL while running the Ruby 1.9.3 test suite on a
> Âstock Debian wheezy installation. ÂWheezy uses tmpfs for "/tmp" by
> Âdefault and the test suite creates a temporary file to test the
> ÂRuby wrapper for posix_fadvise() on.
>
> Âmm/fadvise.c | Â 19 +++++++------------
> Â1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/mm/fadvise.c b/mm/fadvise.c
> index 469491e0..f9e48dd 100644
> --- a/mm/fadvise.c
> +++ b/mm/fadvise.c
> @@ -43,13 +43,13 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, loff_t offset, loff_t len, int advice)
> Â Â Â Â Â Â Â Âgoto out;
> Â Â Â Â}
>
> - Â Â Â mapping = file->f_mapping;
> - Â Â Â if (!mapping || len < 0) {
> + Â Â Â if (len < 0) {

Current code makes sure mapping is valid after the above check,

> Â Â Â Â Â Â Â Âret = -EINVAL;
> Â Â Â Â Â Â Â Âgoto out;
> Â Â Â Â}
>
> - Â Â Â if (mapping->a_ops->get_xip_mem) {
> + Â Â Â mapping = file->f_mapping;
> + Â Â Â if (!mapping || mapping->a_ops->get_xip_mem) {
> Â Â Â Â Â Â Â Âswitch (advice) {
> Â Â Â Â Â Â Â Âcase POSIX_FADV_NORMAL:
> Â Â Â Â Â Â Â Âcase POSIX_FADV_RANDOM:

but backing devices info is no longer evaluated with that
guarantee in your change.

-hd

75:	bdi = mapping->backing_dev_info;

> @@ -93,10 +93,9 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, loff_t offset, loff_t len, int advice)
> Â Â Â Â Â Â Â Âspin_unlock(&file->f_lock);
> Â Â Â Â Â Â Â Âbreak;
> Â Â Â Âcase POSIX_FADV_WILLNEED:
> - Â Â Â Â Â Â Â if (!mapping->a_ops->readpage) {
> - Â Â Â Â Â Â Â Â Â Â Â ret = -EINVAL;
> + Â Â Â Â Â Â Â /* ignore the advice if readahead isn't possible (tmpfs) */
> + Â Â Â Â Â Â Â if (!mapping->a_ops->readpage)
> Â Â Â Â Â Â Â Â Â Â Â Âbreak;
> - Â Â Â Â Â Â Â }
>
> Â Â Â Â Â Â Â Â/* First and last PARTIAL page! */
> Â Â Â Â Â Â Â Âstart_index = offset >> PAGE_CACHE_SHIFT;
> @@ -106,12 +105,8 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, loff_t offset, loff_t len, int advice)
> Â Â Â Â Â Â Â Ânrpages = end_index - start_index + 1;
> Â Â Â Â Â Â Â Âif (!nrpages)
> Â Â Â Â Â Â Â Â Â Â Â Ânrpages = ~0UL;
> -
> - Â Â Â Â Â Â Â ret = force_page_cache_readahead(mapping, file,
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â start_index,
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â nrpages);
> - Â Â Â Â Â Â Â if (ret > 0)
> - Â Â Â Â Â Â Â Â Â Â Â ret = 0;
> +
> + Â Â Â Â Â Â Â force_page_cache_readahead(mapping, file, start_index, nrpages);
> Â Â Â Â Â Â Â Âbreak;
> Â Â Â Âcase POSIX_FADV_NOREUSE:
> Â Â Â Â Â Â Â Âbreak;
> --
> Eric Wong
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at Âhttp://vger.kernel.org/majordomo-info.html
> Please read the FAQ at Âhttp://www.tux.org/lkml/
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/