Good work addressing the Coverity finding. The fix properly eliminates the race condition by moving the check inside the lock. Reviewed-by: Alex Markuze amarkuze@xxxxxxxxxx On Fri, Jun 13, 2025 at 9:35 PM Viacheslav Dubeyko <slava@xxxxxxxxxxx> wrote: > > From: Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx> > > The Coverity Scan service has detected potential > race condition in ceph_ioctl_lazyio() [1]. > > The CID 1591046 contains explanation: "Check of thread-shared > field evades lock acquisition (LOCK_EVASION). Thread1 sets > fmode to a new value. Now the two threads have an inconsistent > view of fmode and updates to fields correlated with fmode > may be lost. The data guarded by this critical section may > be read while in an inconsistent state or modified by multiple > racing threads. In ceph_ioctl_lazyio: Checking the value of > a thread-shared field outside of a locked region to determine > if a locked operation involving that thread shared field > has completed. (CWE-543)". > > The patch places fi->fmode field access under ci->i_ceph_lock > protection. Also, it introduces the is_file_already_lazy > variable that is set under the lock and it is checked later > out of scope of critical section. > > [1] https://scan5.scan.coverity.com/#/project-view/64304/10063?selectedIssue=1591046 > > Signed-off-by: Viacheslav Dubeyko <Slava.Dubeyko@xxxxxxx> > --- > fs/ceph/ioctl.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/fs/ceph/ioctl.c b/fs/ceph/ioctl.c > index e861de3c79b9..60410cf27a34 100644 > --- a/fs/ceph/ioctl.c > +++ b/fs/ceph/ioctl.c > @@ -246,21 +246,27 @@ static long ceph_ioctl_lazyio(struct file *file) > struct ceph_inode_info *ci = ceph_inode(inode); > struct ceph_mds_client *mdsc = ceph_inode_to_fs_client(inode)->mdsc; > struct ceph_client *cl = mdsc->fsc->client; > + bool is_file_already_lazy = false; > > + spin_lock(&ci->i_ceph_lock); > if ((fi->fmode & CEPH_FILE_MODE_LAZY) == 0) { > - spin_lock(&ci->i_ceph_lock); > fi->fmode |= CEPH_FILE_MODE_LAZY; > ci->i_nr_by_mode[ffs(CEPH_FILE_MODE_LAZY)]++; > __ceph_touch_fmode(ci, mdsc, fi->fmode); > - spin_unlock(&ci->i_ceph_lock); > + } else > + is_file_already_lazy = true; > + spin_unlock(&ci->i_ceph_lock); > + > + if (is_file_already_lazy) { > + doutc(cl, "file %p %p %llx.%llx already lazy\n", file, inode, > + ceph_vinop(inode)); > + } else { > doutc(cl, "file %p %p %llx.%llx marked lazy\n", file, inode, > ceph_vinop(inode)); > > ceph_check_caps(ci, 0); > - } else { > - doutc(cl, "file %p %p %llx.%llx already lazy\n", file, inode, > - ceph_vinop(inode)); > } > + > return 0; > } > > -- > 2.49.0 >
