summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorKirill Smelkov <kirr@nexedi.com>2019-04-12 05:31:57 -0400
committerKirill Smelkov <kirr@nexedi.com>2019-05-06 10:46:52 -0400
commit438ab720c675a16d53bb18f76a94d25bbe420c45 (patch)
treef84b1009eb6918a07252b22089dde736cf2ff6d3
parentc5bf68fe0c86a5835bd2e6aead1c49976360753f (diff)
vfs: pass ppos=NULL to .read()/.write() of FMODE_STREAM files
This amends commit 10dce8af3422 ("fs: stream_open - opener for stream-like files so that read and write can run simultaneously without deadlock") in how position is passed into .read()/.write() handler for stream-like files: Rasmus noticed that we currently pass 0 as position and ignore any position change if that is done by a file implementation. This papers over bugs if ppos is used in files that declare themselves as being stream-like as such bugs will go unnoticed. Even if a file implementation is correctly converted into using stream_open, its read/write later could be changed to use ppos and even though that won't be working correctly, that bug might go unnoticed without someone doing wrong behaviour analysis. It is thus better to pass ppos=NULL into read/write for stream-like files as that don't give any chance for ppos usage bugs because it will oops if ppos is ever used inside .read() or .write(). Note 1: rw_verify_area, new_sync_{read,write} needs to be updated because they are called by vfs_read/vfs_write & friends before file_operations .read/.write . Note 2: if file backend uses new-style .read_iter/.write_iter, position is still passed into there as non-pointer kiocb.ki_pos . Currently stream_open.cocci (semantic patch added by 10dce8af3422) ignores files whose file_operations has *_iter methods. Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> Signed-off-by: Kirill Smelkov <kirr@nexedi.com>
-rw-r--r--fs/open.c5
-rw-r--r--fs/read_write.c113
2 files changed, 70 insertions, 48 deletions
diff --git a/fs/open.c b/fs/open.c
index a00350018a47..9c7d724a6f67 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1219,8 +1219,9 @@ EXPORT_SYMBOL(nonseekable_open);
1219/* 1219/*
1220 * stream_open is used by subsystems that want stream-like file descriptors. 1220 * stream_open is used by subsystems that want stream-like file descriptors.
1221 * Such file descriptors are not seekable and don't have notion of position 1221 * Such file descriptors are not seekable and don't have notion of position
1222 * (file.f_pos is always 0). Contrary to file descriptors of other regular 1222 * (file.f_pos is always 0 and ppos passed to .read()/.write() is always NULL).
1223 * files, .read() and .write() can run simultaneously. 1223 * Contrary to file descriptors of other regular files, .read() and .write()
1224 * can run simultaneously.
1224 * 1225 *
1225 * stream_open never fails and is marked to return int so that it could be 1226 * stream_open never fails and is marked to return int so that it could be
1226 * directly used as file_operations.open . 1227 * directly used as file_operations.open .
diff --git a/fs/read_write.c b/fs/read_write.c
index 61b43ad7608e..c543d965e288 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -365,29 +365,37 @@ out_putf:
365int rw_verify_area(int read_write, struct file *file, const loff_t *ppos, size_t count) 365int rw_verify_area(int read_write, struct file *file, const loff_t *ppos, size_t count)
366{ 366{
367 struct inode *inode; 367 struct inode *inode;
368 loff_t pos;
369 int retval = -EINVAL; 368 int retval = -EINVAL;
370 369
371 inode = file_inode(file); 370 inode = file_inode(file);
372 if (unlikely((ssize_t) count < 0)) 371 if (unlikely((ssize_t) count < 0))
373 return retval; 372 return retval;
374 pos = *ppos;
375 if (unlikely(pos < 0)) {
376 if (!unsigned_offsets(file))
377 return retval;
378 if (count >= -pos) /* both values are in 0..LLONG_MAX */
379 return -EOVERFLOW;
380 } else if (unlikely((loff_t) (pos + count) < 0)) {
381 if (!unsigned_offsets(file))
382 return retval;
383 }
384 373
385 if (unlikely(inode->i_flctx && mandatory_lock(inode))) { 374 /*
386 retval = locks_mandatory_area(inode, file, pos, pos + count - 1, 375 * ranged mandatory locking does not apply to streams - it makes sense
387 read_write == READ ? F_RDLCK : F_WRLCK); 376 * only for files where position has a meaning.
388 if (retval < 0) 377 */
389 return retval; 378 if (ppos) {
379 loff_t pos = *ppos;
380
381 if (unlikely(pos < 0)) {
382 if (!unsigned_offsets(file))
383 return retval;
384 if (count >= -pos) /* both values are in 0..LLONG_MAX */
385 return -EOVERFLOW;
386 } else if (unlikely((loff_t) (pos + count) < 0)) {
387 if (!unsigned_offsets(file))
388 return retval;
389 }
390
391 if (unlikely(inode->i_flctx && mandatory_lock(inode))) {
392 retval = locks_mandatory_area(inode, file, pos, pos + count - 1,
393 read_write == READ ? F_RDLCK : F_WRLCK);
394 if (retval < 0)
395 return retval;
396 }
390 } 397 }
398
391 return security_file_permission(file, 399 return security_file_permission(file,
392 read_write == READ ? MAY_READ : MAY_WRITE); 400 read_write == READ ? MAY_READ : MAY_WRITE);
393} 401}
@@ -400,12 +408,13 @@ static ssize_t new_sync_read(struct file *filp, char __user *buf, size_t len, lo
400 ssize_t ret; 408 ssize_t ret;
401 409
402 init_sync_kiocb(&kiocb, filp); 410 init_sync_kiocb(&kiocb, filp);
403 kiocb.ki_pos = *ppos; 411 kiocb.ki_pos = (ppos ? *ppos : 0);
404 iov_iter_init(&iter, READ, &iov, 1, len); 412 iov_iter_init(&iter, READ, &iov, 1, len);
405 413
406 ret = call_read_iter(filp, &kiocb, &iter); 414 ret = call_read_iter(filp, &kiocb, &iter);
407 BUG_ON(ret == -EIOCBQUEUED); 415 BUG_ON(ret == -EIOCBQUEUED);
408 *ppos = kiocb.ki_pos; 416 if (ppos)
417 *ppos = kiocb.ki_pos;
409 return ret; 418 return ret;
410} 419}
411 420
@@ -468,12 +477,12 @@ static ssize_t new_sync_write(struct file *filp, const char __user *buf, size_t
468 ssize_t ret; 477 ssize_t ret;
469 478
470 init_sync_kiocb(&kiocb, filp); 479 init_sync_kiocb(&kiocb, filp);
471 kiocb.ki_pos = *ppos; 480 kiocb.ki_pos = (ppos ? *ppos : 0);
472 iov_iter_init(&iter, WRITE, &iov, 1, len); 481 iov_iter_init(&iter, WRITE, &iov, 1, len);
473 482
474 ret = call_write_iter(filp, &kiocb, &iter); 483 ret = call_write_iter(filp, &kiocb, &iter);
475 BUG_ON(ret == -EIOCBQUEUED); 484 BUG_ON(ret == -EIOCBQUEUED);
476 if (ret > 0) 485 if (ret > 0 && ppos)
477 *ppos = kiocb.ki_pos; 486 *ppos = kiocb.ki_pos;
478 return ret; 487 return ret;
479} 488}
@@ -558,15 +567,10 @@ ssize_t vfs_write(struct file *file, const char __user *buf, size_t count, loff_
558 return ret; 567 return ret;
559} 568}
560 569
561static inline loff_t file_pos_read(struct file *file) 570/* file_ppos returns &file->f_pos or NULL if file is stream */
562{ 571static inline loff_t *file_ppos(struct file *file)
563 return file->f_mode & FMODE_STREAM ? 0 : file->f_pos;
564}
565
566static inline void file_pos_write(struct file *file, loff_t pos)
567{ 572{
568 if ((file->f_mode & FMODE_STREAM) == 0) 573 return file->f_mode & FMODE_STREAM ? NULL : &file->f_pos;
569 file->f_pos = pos;
570} 574}
571 575
572ssize_t ksys_read(unsigned int fd, char __user *buf, size_t count) 576ssize_t ksys_read(unsigned int fd, char __user *buf, size_t count)
@@ -575,10 +579,14 @@ ssize_t ksys_read(unsigned int fd, char __user *buf, size_t count)
575 ssize_t ret = -EBADF; 579 ssize_t ret = -EBADF;
576 580
577 if (f.file) { 581 if (f.file) {
578 loff_t pos = file_pos_read(f.file); 582 loff_t pos, *ppos = file_ppos(f.file);
579 ret = vfs_read(f.file, buf, count, &pos); 583 if (ppos) {
580 if (ret >= 0) 584 pos = *ppos;
581 file_pos_write(f.file, pos); 585 ppos = &pos;
586 }
587 ret = vfs_read(f.file, buf, count, ppos);
588 if (ret >= 0 && ppos)
589 f.file->f_pos = pos;
582 fdput_pos(f); 590 fdput_pos(f);
583 } 591 }
584 return ret; 592 return ret;
@@ -595,10 +603,14 @@ ssize_t ksys_write(unsigned int fd, const char __user *buf, size_t count)
595 ssize_t ret = -EBADF; 603 ssize_t ret = -EBADF;
596 604
597 if (f.file) { 605 if (f.file) {
598 loff_t pos = file_pos_read(f.file); 606 loff_t pos, *ppos = file_ppos(f.file);
599 ret = vfs_write(f.file, buf, count, &pos); 607 if (ppos) {
600 if (ret >= 0) 608 pos = *ppos;
601 file_pos_write(f.file, pos); 609 ppos = &pos;
610 }
611 ret = vfs_write(f.file, buf, count, ppos);
612 if (ret >= 0 && ppos)
613 f.file->f_pos = pos;
602 fdput_pos(f); 614 fdput_pos(f);
603 } 615 }
604 616
@@ -673,14 +685,15 @@ static ssize_t do_iter_readv_writev(struct file *filp, struct iov_iter *iter,
673 ret = kiocb_set_rw_flags(&kiocb, flags); 685 ret = kiocb_set_rw_flags(&kiocb, flags);
674 if (ret) 686 if (ret)
675 return ret; 687 return ret;
676 kiocb.ki_pos = *ppos; 688 kiocb.ki_pos = (ppos ? *ppos : 0);
677 689
678 if (type == READ) 690 if (type == READ)
679 ret = call_read_iter(filp, &kiocb, iter); 691 ret = call_read_iter(filp, &kiocb, iter);
680 else 692 else
681 ret = call_write_iter(filp, &kiocb, iter); 693 ret = call_write_iter(filp, &kiocb, iter);
682 BUG_ON(ret == -EIOCBQUEUED); 694 BUG_ON(ret == -EIOCBQUEUED);
683 *ppos = kiocb.ki_pos; 695 if (ppos)
696 *ppos = kiocb.ki_pos;
684 return ret; 697 return ret;
685} 698}
686 699
@@ -1013,10 +1026,14 @@ static ssize_t do_readv(unsigned long fd, const struct iovec __user *vec,
1013 ssize_t ret = -EBADF; 1026 ssize_t ret = -EBADF;
1014 1027
1015 if (f.file) { 1028 if (f.file) {
1016 loff_t pos = file_pos_read(f.file); 1029 loff_t pos, *ppos = file_ppos(f.file);
1017 ret = vfs_readv(f.file, vec, vlen, &pos, flags); 1030 if (ppos) {
1018 if (ret >= 0) 1031 pos = *ppos;
1019 file_pos_write(f.file, pos); 1032 ppos = &pos;
1033 }
1034 ret = vfs_readv(f.file, vec, vlen, ppos, flags);
1035 if (ret >= 0 && ppos)
1036 f.file->f_pos = pos;
1020 fdput_pos(f); 1037 fdput_pos(f);
1021 } 1038 }
1022 1039
@@ -1033,10 +1050,14 @@ static ssize_t do_writev(unsigned long fd, const struct iovec __user *vec,
1033 ssize_t ret = -EBADF; 1050 ssize_t ret = -EBADF;
1034 1051
1035 if (f.file) { 1052 if (f.file) {
1036 loff_t pos = file_pos_read(f.file); 1053 loff_t pos, *ppos = file_ppos(f.file);
1037 ret = vfs_writev(f.file, vec, vlen, &pos, flags); 1054 if (ppos) {
1038 if (ret >= 0) 1055 pos = *ppos;
1039 file_pos_write(f.file, pos); 1056 ppos = &pos;
1057 }
1058 ret = vfs_writev(f.file, vec, vlen, ppos, flags);
1059 if (ret >= 0 && ppos)
1060 f.file->f_pos = pos;
1040 fdput_pos(f); 1061 fdput_pos(f);
1041 } 1062 }
1042 1063