Re: [RESEND] [PATCH] VFS: make file->f_pos access atomic on 32bit arch

Linus Torvalds
2008-10-09T15:00:32Z

On Thu, 9 Oct 2008, Matthew Wilcox wrote:
> On Thu, Oct 09, 2008 at 02:23:19PM +0200, Pavel Machek wrote:
> >
> > We have append-only files, and normal users should not be able to work
> > around that restriction.
>
> Is it possible to work around this restriction by exploiting this?

No, I don't think it is.

Because we had various nasty races, and various broken filesystems using
"f->f_pos" directly (and then pread/pwrite not working), we fixed things
many years ago, and nobody should use "f_pos" directly any more for any
regular file access.

Oh, you'll see a _lot_ of f_pos accesses if you grep for them in low-level
filesystems, but they should be for directory accesses, that are all under
i_mutex. And O_APPEND obviously doesn't enter into it anyway.

So for regular IO, all the filesystems should never touch f_pos directly
at all, they only ever touch a local "pos" that gets cached, and then at
the end of the write sys_write() will write it back with file_pos_write().
That function was done exactly so that we _could_ do locking if we cared.
Nobody ever did.

So even though filesystems get passed a _pointer_ to the position, it's
all actually a pointer to just a private per-thread, on-stack entry.

The reason for that is that we really used to have bugs where the
low-level filesystem assumed that "*pos" didn't change from under it while
the access was going on (reading it multiple times and comparing against
i_size etc), and exactly due to things like O_APPEND races against lseek.

So I think f_pos is fine. Yes, yes, if two threads or processes access the
same file pointer concurrently, that means that f_pos at the end may be
crazy, but that really is true regardless of whether you are able to hit
the *very* small race of updating the 32-bit lower/upper fields in some
mixed manner. No sane user program can possibly really care, since it
would already be getting random offsets.

(Yeah, yeah, I could see some really crazy code that can do retries with
optimistic locking in user space and could possibly see this as a bug, but
that really is totally insane code, and I doubt you could write such a
crazy thing to actually work).

Linus