Messages in this thread Patch in this message | | | Date | Thu, 13 Apr 2006 08:28:27 -0700 (PDT) | From | Linus Torvalds <> | Subject | Re: PROBLEM: pthread-safety bug in write(2) on Linux 2.6.x |
| |
On Thu, 13 Apr 2006, Linus Torvalds wrote: > > That said, I wouldn't be 100% against it, especially under certain > circumstances. However, the circumstances when I think it might be > acceptable are fairly specific: > > - when we use f_pos (ie we'd synchronize write against write, but only > per "struct file", not on an inode basis) > - only for files that are marked seekable with FMODE_LSEEK (thus avoiding > the stream-like objects like pipes and sockets) > > Under those two circumstances, I'd certainly be ok with it, and I think we > could argue that it is a "good thing". It would be a "f_pos" lock (so we > migt do it for reads too), not a "data lock". > > Comments?
Something like this.
NOTE NOTE NOTE! Untested!
Linus --- diff --git a/fs/file_table.c b/fs/file_table.c index bcea199..48728cc 100644 --- a/fs/file_table.c +++ b/fs/file_table.c @@ -120,6 +120,7 @@ struct file *get_empty_filp(void) f->f_uid = tsk->fsuid; f->f_gid = tsk->fsgid; eventpoll_init_file(f); + mutex_init(&f->f_pos_lock); /* f->f_version: 0 */ return f; diff --git a/fs/read_write.c b/fs/read_write.c index 5bc0e92..ad9db26 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -329,14 +329,23 @@ ssize_t vfs_write(struct file *file, con EXPORT_SYMBOL(vfs_write); -static inline loff_t file_pos_read(struct file *file) +/* + * FMODE_LSEEK will never change for a file during its lifetime, + * so it's ok to test it independently on the lock/unlock path + * rather than explicitly remembering whether we locked it. + */ +static inline loff_t file_pos_read_lock(struct file *file) { + if (file->f_mode & FMODE_LSEEK) + mutex_lock(&file->f_pos_lock); return file->f_pos; } -static inline void file_pos_write(struct file *file, loff_t pos) +static inline void file_pos_write_unlock(struct file *file, loff_t pos) { file->f_pos = pos; + if (file->f_mode & FMODE_LSEEK) + mutex_unlock(&file->f_pos_lock); } asmlinkage ssize_t sys_read(unsigned int fd, char __user * buf, size_t count) @@ -347,9 +356,9 @@ asmlinkage ssize_t sys_read(unsigned int file = fget_light(fd, &fput_needed); if (file) { - loff_t pos = file_pos_read(file); + loff_t pos = file_pos_read_lock(file); ret = vfs_read(file, buf, count, &pos); - file_pos_write(file, pos); + file_pos_write_unlock(file, pos); fput_light(file, fput_needed); } @@ -365,9 +374,9 @@ asmlinkage ssize_t sys_write(unsigned in file = fget_light(fd, &fput_needed); if (file) { - loff_t pos = file_pos_read(file); + loff_t pos = file_pos_read_lock(file); ret = vfs_write(file, buf, count, &pos); - file_pos_write(file, pos); + file_pos_write_unlock(file, pos); fput_light(file, fput_needed); } @@ -603,9 +612,9 @@ sys_readv(unsigned long fd, const struct file = fget_light(fd, &fput_needed); if (file) { - loff_t pos = file_pos_read(file); + loff_t pos = file_pos_read_lock(file); ret = vfs_readv(file, vec, vlen, &pos); - file_pos_write(file, pos); + file_pos_write_unlock(file, pos); fput_light(file, fput_needed); } @@ -624,9 +633,9 @@ sys_writev(unsigned long fd, const struc file = fget_light(fd, &fput_needed); if (file) { - loff_t pos = file_pos_read(file); + loff_t pos = file_pos_read_lock(file); ret = vfs_writev(file, vec, vlen, &pos); - file_pos_write(file, pos); + file_pos_write_unlock(file, pos); fput_light(file, fput_needed); } diff --git a/include/linux/fs.h b/include/linux/fs.h index 162c6e5..1d58cd0 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -643,6 +643,7 @@ struct file { loff_t f_pos; struct fown_struct f_owner; unsigned int f_uid, f_gid; + struct mutex f_pos_lock; struct file_ra_state f_ra; unsigned long f_version; - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |