| Message ID | 20210506184304.321645-2-vgoyal@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtiofs, dax: Fix couple of smatch warnings | expand |
* Vivek Goyal (vgoyal@redhat.com) wrote: > Dan reported a smatch warning during potentential loss of info during > left shift if this code is compiled on 32bit systems. > > New smatch warnings: > fs/fuse/dax.c:113 fuse_setup_one_mapping() warn: should 'start_idx << 21' be a > +64 bit type? > > I ran smatch and found two more instances of similar warning. This patch > fixes all such instances. > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- > fs/fuse/dax.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c > index ff99ab2a3c43..f06fdad3f7b1 100644 > --- a/fs/fuse/dax.c > +++ b/fs/fuse/dax.c > @@ -186,7 +186,7 @@ static int fuse_setup_one_mapping(struct inode *inode, unsigned long start_idx, > struct fuse_conn_dax *fcd = fm->fc->dax; > struct fuse_inode *fi = get_fuse_inode(inode); > struct fuse_setupmapping_in inarg; > - loff_t offset = start_idx << FUSE_DAX_SHIFT; > + loff_t offset = (loff_t)start_idx << FUSE_DAX_SHIFT; I've not followed the others back, but isn't it easier to change the start_idx parameter to be a loff_t, since the places it's called from are poth loff_t pos? Dave > FUSE_ARGS(args); > ssize_t err; > > @@ -872,7 +872,7 @@ static int dmap_writeback_invalidate(struct inode *inode, > struct fuse_dax_mapping *dmap) > { > int ret; > - loff_t start_pos = dmap->itn.start << FUSE_DAX_SHIFT; > + loff_t start_pos = (loff_t)dmap->itn.start << FUSE_DAX_SHIFT; > loff_t end_pos = (start_pos + FUSE_DAX_SZ - 1); > > ret = filemap_fdatawrite_range(inode->i_mapping, start_pos, end_pos); > @@ -966,7 +966,7 @@ inode_inline_reclaim_one_dmap(struct fuse_conn_dax *fcd, struct inode *inode, > dmap = inode_lookup_first_dmap(inode); > if (dmap) { > start_idx = dmap->itn.start; > - dmap_start = start_idx << FUSE_DAX_SHIFT; > + dmap_start = (u64)start_idx << FUSE_DAX_SHIFT; > dmap_end = dmap_start + FUSE_DAX_SZ - 1; > } > up_read(&fi->dax->sem); > @@ -1118,7 +1118,7 @@ static int lookup_and_reclaim_dmap(struct fuse_conn_dax *fcd, > { > int ret; > struct fuse_inode *fi = get_fuse_inode(inode); > - loff_t dmap_start = start_idx << FUSE_DAX_SHIFT; > + loff_t dmap_start = (loff_t)start_idx << FUSE_DAX_SHIFT; > loff_t dmap_end = (dmap_start + FUSE_DAX_SZ) - 1; > > down_write(&fi->i_mmap_sem); > -- > 2.25.4 >
On Thu, May 06, 2021 at 08:07:39PM +0100, Dr. David Alan Gilbert wrote: > > @@ -186,7 +186,7 @@ static int fuse_setup_one_mapping(struct inode *inode, unsigned long start_idx, > > struct fuse_conn_dax *fcd = fm->fc->dax; > > struct fuse_inode *fi = get_fuse_inode(inode); > > struct fuse_setupmapping_in inarg; > > - loff_t offset = start_idx << FUSE_DAX_SHIFT; > > + loff_t offset = (loff_t)start_idx << FUSE_DAX_SHIFT; > > I've not followed the others back, but isn't it easier to change > the start_idx parameter to be a loff_t, since the places it's called > from are poth loff_t pos? But an index isn't a file offset, and shouldn't be typed as such.
On Thu, May 06, 2021 at 12:35:17PM -0700, Matthew Wilcox wrote: > On Thu, May 06, 2021 at 08:07:39PM +0100, Dr. David Alan Gilbert wrote: > > > @@ -186,7 +186,7 @@ static int fuse_setup_one_mapping(struct inode *inode, unsigned long start_idx, > > > struct fuse_conn_dax *fcd = fm->fc->dax; > > > struct fuse_inode *fi = get_fuse_inode(inode); > > > struct fuse_setupmapping_in inarg; > > > - loff_t offset = start_idx << FUSE_DAX_SHIFT; > > > + loff_t offset = (loff_t)start_idx << FUSE_DAX_SHIFT; > > > > I've not followed the others back, but isn't it easier to change > > the start_idx parameter to be a loff_t, since the places it's called > > from are poth loff_t pos? > > But an index isn't a file offset, and shouldn't be typed as such. Agreed. This is index, so it seems better to not use "loff_t" to represent it. Vivek
diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c index ff99ab2a3c43..f06fdad3f7b1 100644 --- a/fs/fuse/dax.c +++ b/fs/fuse/dax.c @@ -186,7 +186,7 @@ static int fuse_setup_one_mapping(struct inode *inode, unsigned long start_idx, struct fuse_conn_dax *fcd = fm->fc->dax; struct fuse_inode *fi = get_fuse_inode(inode); struct fuse_setupmapping_in inarg; - loff_t offset = start_idx << FUSE_DAX_SHIFT; + loff_t offset = (loff_t)start_idx << FUSE_DAX_SHIFT; FUSE_ARGS(args); ssize_t err; @@ -872,7 +872,7 @@ static int dmap_writeback_invalidate(struct inode *inode, struct fuse_dax_mapping *dmap) { int ret; - loff_t start_pos = dmap->itn.start << FUSE_DAX_SHIFT; + loff_t start_pos = (loff_t)dmap->itn.start << FUSE_DAX_SHIFT; loff_t end_pos = (start_pos + FUSE_DAX_SZ - 1); ret = filemap_fdatawrite_range(inode->i_mapping, start_pos, end_pos); @@ -966,7 +966,7 @@ inode_inline_reclaim_one_dmap(struct fuse_conn_dax *fcd, struct inode *inode, dmap = inode_lookup_first_dmap(inode); if (dmap) { start_idx = dmap->itn.start; - dmap_start = start_idx << FUSE_DAX_SHIFT; + dmap_start = (u64)start_idx << FUSE_DAX_SHIFT; dmap_end = dmap_start + FUSE_DAX_SZ - 1; } up_read(&fi->dax->sem); @@ -1118,7 +1118,7 @@ static int lookup_and_reclaim_dmap(struct fuse_conn_dax *fcd, { int ret; struct fuse_inode *fi = get_fuse_inode(inode); - loff_t dmap_start = start_idx << FUSE_DAX_SHIFT; + loff_t dmap_start = (loff_t)start_idx << FUSE_DAX_SHIFT; loff_t dmap_end = (dmap_start + FUSE_DAX_SZ) - 1; down_write(&fi->i_mmap_sem);
Dan reported a smatch warning during potentential loss of info during left shift if this code is compiled on 32bit systems. New smatch warnings: fs/fuse/dax.c:113 fuse_setup_one_mapping() warn: should 'start_idx << 21' be a +64 bit type? I ran smatch and found two more instances of similar warning. This patch fixes all such instances. Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- fs/fuse/dax.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)