Created attachment 1840453 [details]
Patch to set desc.error so the error code gets passed back up.

Description of problem:

generic_file_aio_read (or generic_file_aio_read2 for ext4) is used by many filesystems, directly or indirectly, to implement their read or aio_read vfs file operations.

Here is the part where it calls do_generic_file_read, a void function, passing it a pointer to the 'desc' structure.  Note that desc.written and desc.error are initialized to 0:

                desc.written = 0;
                desc.arg.buf = iov[seg].iov_base + offset;
                desc.count = iov[seg].iov_len - offset;
                if (desc.count == 0)
                        continue;
                desc.error = 0;
                do_generic_file_read(filp, ppos, &desc, file_read_actor);
                retval += desc.written;
                if (desc.error) {
                        retval = retval ?: desc.error;
                        break;
                }

In do_generic_file_read, there is a check for a pending fatal signal:

                if (fatal_signal_pending(current)) {
                        error = -EINTR;
                        goto out;
                }

and at the out label:

out:
        ra->prev_pos = prev_index;
        ra->prev_pos <<= PAGE_CACHE_SHIFT;
        ra->prev_pos |= prev_offset;

        *ppos = ((loff_t)index << PAGE_CACHE_SHIFT) + offset;
        file_accessed(filp);
}

Note that the local variable 'error' is not returned or assigned to anything, so the -EINTR is lost and never makes it to the caller.  If no data has been read yet (retval == 0), the function will just return 0, since desc.error is 0.

This means that many file systems can return 0 bytes from ->read, even though the request is not at EOF and some data might be available.

Upstream in the mainline kernel the fatal signal check was introduced by this commit:

commit 5abf186a30a89d5b9c18a6bf93a2c192c9fd52f6
Author: Michal Hocko <mhocko>
Date:   Fri Feb 3 13:13:29 2017 -0800

    mm, fs: check for fatal signals in do_generic_file_read()

But at that point in the mainline kernel, the function was not void and the out label had:

    return written ? written : error;

so the -EINTR was correctly passed back up to callers.  This looks like an error in the backporting of that patch to 3.10; the same can be seen in the 3.10 stable kernel in 3.10.107 and 3.10.108.

This is probably not noticeable when the read is initiated from userspace through a syscall, as the process is terminated because of the fatal signal, and the error code doesn't matter.

It can however cause problems for in kernel callers that call ->read or ->aio_read directly and assume that a return of 0 indicates that there is no data to be read, as is usually guaranteed.

There's evidence that this can occur with AuriStorFS clients, when the kernel module accesses files that are part of the local disk cache.  Problems have been reported with accesses to the disk cache index file, and data files used to store directory contents.


Version-Release number of selected component (if applicable):

The problem was observed with kernel 3.10.0-1160.31.1.el7.x86_64.  Code inspection suggests it was introduced during the cycle leading up to 3.10.0-862.el7.


How reproducible:

The problem has been observed regularly (~10 times a day) in a large set of servers running the AuriStorFS client.


Steps to Reproduce:

I will see if I can come up with a script/scenario that can reproduce the problem on demand.



Actual results:


Expected results:


Additional info:

See attached proposed patch.