| Message ID | CO1PR15MB4876E2C9F48D78426091DCD092332@CO1PR15MB4876.namprd15.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | ceph: improve error handling and short/overflow-read logic in __ceph_sync_read() | expand |
On Sun, 2024-12-08 at 17:32 +0000, Alex Markuze wrote: > This patch refines the read logic in __ceph_sync_read() to ensure > more > predictable and efficient behavior in various edge cases. > > - Return early if the requested read length is zero or if the file > size > (`i_size`) is zero. > - Initialize the index variable (`idx`) where needed and reorder some > code to ensure it is always set before use. > - Improve error handling by checking for negative return values > earlier. > - Remove redundant encrypted file checks after failures. Only attempt > filesystem-level decryption if the read succeeded. > - Simplify leftover calculations to correctly handle cases where the > read > extends beyond the end of the file or stops short. > - This resolves multiple issues caused by integer overflow > - > https://tracker.ceph.com/issues/67524 > > - > https://tracker.ceph.com/issues/68981 > > - > https://tracker.ceph.com/issues/68980 > > > Signed-off-by: Alex Markuze <amarkuze@redhat.com> > --- > fs/ceph/file.c | 29 ++++++++++++++--------------- > 1 file changed, 14 insertions(+), 15 deletions(-) > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > index ce342a5d4b8b..8e0400d461a2 100644 > --- a/fs/ceph/file.c > +++ b/fs/ceph/file.c > @@ -1066,7 +1066,7 @@ ssize_t __ceph_sync_read(struct inode *inode, > loff_t *ki_pos, > if (ceph_inode_is_shutdown(inode)) > return -EIO; > > - if (!len) > + if (!len || !i_size) > return 0; > /* > * flush any page cache pages in this range. this > @@ -1086,7 +1086,7 @@ ssize_t __ceph_sync_read(struct inode *inode, > loff_t *ki_pos, > int num_pages; > size_t page_off; > bool more; > - int idx; > + int idx = 0; > size_t left; > struct ceph_osd_req_op *op; > u64 read_off = off; > @@ -1160,7 +1160,14 @@ ssize_t __ceph_sync_read(struct inode *inode, > loff_t *ki_pos, > else if (ret == -ENOENT) > ret = 0; > > - if (ret > 0 && IS_ENCRYPTED(inode)) { > + if (ret < 0) { If I understood correctly, you simply added the check for error in the execution flow (ret < 0) before processing IS_ENCRYPTED(inode) condition. Am I correct here? Thanks, Slava. > + ceph_osdc_put_request(req); > + if (ret == -EBLOCKLISTED) > + fsc->blocklisted = true; > + break; > + } > + > + if (IS_ENCRYPTED(inode)) { > int fret; > > fret = ceph_fscrypt_decrypt_extents(inode, > pages, > @@ -1187,7 +1194,7 @@ ssize_t __ceph_sync_read(struct inode *inode, > loff_t *ki_pos, > } > > /* Short read but not EOF? Zero out the remainder. > */ > - if (ret >= 0 && ret < len && (off + ret < i_size)) { > + if (ret < len && (off + ret < i_size)) { > int zlen = min(len - ret, i_size - off - > ret); > int zoff = page_off + ret; > > @@ -1197,13 +1204,11 @@ ssize_t __ceph_sync_read(struct inode *inode, > loff_t *ki_pos, > ret += zlen; > } > > - idx = 0; > - if (ret <= 0) > - left = 0; > - else if (off + ret > i_size) > - left = i_size - off; > + if (off + ret > i_size) > + left = (i_size > off) ? i_size - off : 0; > else > left = ret; > + > while (left > 0) { > size_t plen, copied; > > @@ -1222,12 +1227,6 @@ ssize_t __ceph_sync_read(struct inode *inode, > loff_t *ki_pos, > > ceph_osdc_put_request(req); > > - if (ret < 0) { > - if (ret == -EBLOCKLISTED) > - fsc->blocklisted = true; > - break; > - } > - > if (off >= i_size || !more) > break; > } > -- > 2.34.1
diff --git a/fs/ceph/file.c b/fs/ceph/file.c index ce342a5d4b8b..8e0400d461a2 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -1066,7 +1066,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, if (ceph_inode_is_shutdown(inode)) return -EIO; - if (!len) + if (!len || !i_size) return 0; /* * flush any page cache pages in this range. this @@ -1086,7 +1086,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, int num_pages; size_t page_off; bool more; - int idx; + int idx = 0; size_t left; struct ceph_osd_req_op *op; u64 read_off = off; @@ -1160,7 +1160,14 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, else if (ret == -ENOENT) ret = 0; - if (ret > 0 && IS_ENCRYPTED(inode)) { + if (ret < 0) { + ceph_osdc_put_request(req); + if (ret == -EBLOCKLISTED) + fsc->blocklisted = true; + break; + } + + if (IS_ENCRYPTED(inode)) { int fret; fret = ceph_fscrypt_decrypt_extents(inode, pages, @@ -1187,7 +1194,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, } /* Short read but not EOF? Zero out the remainder. */ - if (ret >= 0 && ret < len && (off + ret < i_size)) { + if (ret < len && (off + ret < i_size)) { int zlen = min(len - ret, i_size - off - ret); int zoff = page_off + ret; @@ -1197,13 +1204,11 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, ret += zlen; } - idx = 0; - if (ret <= 0) - left = 0; - else if (off + ret > i_size) - left = i_size - off; + if (off + ret > i_size) + left = (i_size > off) ? i_size - off : 0; else left = ret; + while (left > 0) { size_t plen, copied; @@ -1222,12 +1227,6 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, ceph_osdc_put_request(req); - if (ret < 0) { - if (ret == -EBLOCKLISTED) - fsc->blocklisted = true; - break; - } - if (off >= i_size || !more) break; }
This patch refines the read logic in __ceph_sync_read() to ensure more predictable and efficient behavior in various edge cases. - Return early if the requested read length is zero or if the file size (`i_size`) is zero. - Initialize the index variable (`idx`) where needed and reorder some code to ensure it is always set before use. - Improve error handling by checking for negative return values earlier. - Remove redundant encrypted file checks after failures. Only attempt filesystem-level decryption if the read succeeded. - Simplify leftover calculations to correctly handle cases where the read extends beyond the end of the file or stops short. - This resolves multiple issues caused by integer overflow - https://tracker.ceph.com/issues/67524 - https://tracker.ceph.com/issues/68981 - https://tracker.ceph.com/issues/68980 Signed-off-by: Alex Markuze <amarkuze@redhat.com> --- fs/ceph/file.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) -- 2.34.1