| Message ID | YpiWS/WQr2qMidvA@kili |
|---|---|
| State | New |
| Headers | show |
| Series | ksmbd: clean up a type in ksmbd_vfs_stream_write() | expand |
2022-06-02 19:51 GMT+09:00, Dan Carpenter <dan.carpenter@oracle.com>: > The existing code works fine because ksmbd_vfs_getcasexattr() isn't > going to return values greater than INT_MAX. But it's ugly to do > the casting and using a ssize_t makes everything cleaner. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > fs/ksmbd/vfs.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/fs/ksmbd/vfs.c b/fs/ksmbd/vfs.c > index dcdd07c6efff..efdc35717f6d 100644 > --- a/fs/ksmbd/vfs.c > +++ b/fs/ksmbd/vfs.c > @@ -411,7 +411,8 @@ static int ksmbd_vfs_stream_write(struct ksmbd_file *fp, > char *buf, loff_t *pos, > { > char *stream_buf = NULL, *wbuf; > struct user_namespace *user_ns = file_mnt_user_ns(fp->filp); > - size_t size, v_len; > + ssize_t v_len; > + size_t size; > int err = 0; > > ksmbd_debug(VFS, "write stream data pos : %llu, count : %zd\n", > @@ -428,9 +429,9 @@ static int ksmbd_vfs_stream_write(struct ksmbd_file *fp, > char *buf, loff_t *pos, > fp->stream.name, > fp->stream.size, > &stream_buf); > - if ((int)v_len < 0) { > + if (v_len < 0) { > pr_err("not found stream in xattr : %zd\n", v_len); > - err = (int)v_len; > + err = v_len; Data type of ssize_t is long. Wouldn't some static checker warn us that this is a problem? Thanks! > goto out; > } > > -- > 2.35.1 > >
On Fri, Jun 03, 2022 at 08:18:19AM +0900, Namjae Jeon wrote: > 2022-06-02 19:51 GMT+09:00, Dan Carpenter <dan.carpenter@oracle.com>: > > @@ -428,9 +429,9 @@ static int ksmbd_vfs_stream_write(struct ksmbd_file *fp, > > char *buf, loff_t *pos, > > fp->stream.name, > > fp->stream.size, > > &stream_buf); > > - if ((int)v_len < 0) { > > + if (v_len < 0) { > > pr_err("not found stream in xattr : %zd\n", v_len); > > - err = (int)v_len; > > + err = v_len; > Data type of ssize_t is long. Wouldn't some static checker warn us > that this is a problem? None that I know of. To a human reader, the cast isn't needed because when a function returns ssize_t the negatives can only be error codes in the (-4095)-(-1) range. No other negative sizes make sense. On the other hand, the "if ((int)v_len < 0) {" line really should trigger a static checker because there are times when sizes could be over 2GB. I will write down that I need to create that checker. regards, dan carpenter
2022-06-03 9:10 GMT+09:00, Dan Carpenter <dan.carpenter@oracle.com>: > On Fri, Jun 03, 2022 at 08:18:19AM +0900, Namjae Jeon wrote: >> 2022-06-02 19:51 GMT+09:00, Dan Carpenter <dan.carpenter@oracle.com>: >> > @@ -428,9 +429,9 @@ static int ksmbd_vfs_stream_write(struct ksmbd_file >> > *fp, >> > char *buf, loff_t *pos, >> > fp->stream.name, >> > fp->stream.size, >> > &stream_buf); >> > - if ((int)v_len < 0) { >> > + if (v_len < 0) { >> > pr_err("not found stream in xattr : %zd\n", v_len); >> > - err = (int)v_len; >> > + err = v_len; >> Data type of ssize_t is long. Wouldn't some static checker warn us >> that this is a problem? > > None that I know of. > > To a human reader, the cast isn't needed because when a function returns > ssize_t the negatives can only be error codes in the (-4095)-(-1) range. > No other negative sizes make sense. > > On the other hand, the "if ((int)v_len < 0) {" line really should > trigger a static checker because there are times when sizes could be > over 2GB. I will write down that I need to create that checker. Okay, And there is a similar type casting in ksmbd_vfs_stream_read(). Is it not necessary to change together? > > regards, > dan carpenter > >
On Fri, Jun 03, 2022 at 09:45:21AM +0900, Namjae Jeon wrote: > 2022-06-03 9:10 GMT+09:00, Dan Carpenter <dan.carpenter@oracle.com>: > > On Fri, Jun 03, 2022 at 08:18:19AM +0900, Namjae Jeon wrote: > >> 2022-06-02 19:51 GMT+09:00, Dan Carpenter <dan.carpenter@oracle.com>: > >> > @@ -428,9 +429,9 @@ static int ksmbd_vfs_stream_write(struct ksmbd_file > >> > *fp, > >> > char *buf, loff_t *pos, > >> > fp->stream.name, > >> > fp->stream.size, > >> > &stream_buf); > >> > - if ((int)v_len < 0) { > >> > + if (v_len < 0) { > >> > pr_err("not found stream in xattr : %zd\n", v_len); > >> > - err = (int)v_len; > >> > + err = v_len; > >> Data type of ssize_t is long. Wouldn't some static checker warn us > >> that this is a problem? > > > > None that I know of. > > > > To a human reader, the cast isn't needed because when a function returns > > ssize_t the negatives can only be error codes in the (-4095)-(-1) range. > > No other negative sizes make sense. > > > > On the other hand, the "if ((int)v_len < 0) {" line really should > > trigger a static checker because there are times when sizes could be > > over 2GB. I will write down that I need to create that checker. > Okay, And there is a similar type casting in ksmbd_vfs_stream_read(). > Is it not necessary to change together? The motivation for this change was that I was looking for places which save negative error codes in a positive value. Since the ksmbd_vfs_stream_read() is using ssize_t then it didn't trigger that warning. But ksmbd_vfs_stream_read() has some minor issues. if ((int)v_len <= 0) This cast is "buggy". Harmless though. return (int)v_len; This is fine, but unnecessary. if (v_len <= *pos) { count = -EINVAL; I feel like this should return 0 bytes instead of -EINVAL; It probably doesn't matter. I am going to try write a static checker that complains specifically about casting size_t (not long) to int. I will make it complain about ssize_t as well, but that's kind of unusual because ssize_t can already store negatives. regards, dan carpenter
diff --git a/fs/ksmbd/vfs.c b/fs/ksmbd/vfs.c index dcdd07c6efff..efdc35717f6d 100644 --- a/fs/ksmbd/vfs.c +++ b/fs/ksmbd/vfs.c @@ -411,7 +411,8 @@ static int ksmbd_vfs_stream_write(struct ksmbd_file *fp, char *buf, loff_t *pos, { char *stream_buf = NULL, *wbuf; struct user_namespace *user_ns = file_mnt_user_ns(fp->filp); - size_t size, v_len; + ssize_t v_len; + size_t size; int err = 0; ksmbd_debug(VFS, "write stream data pos : %llu, count : %zd\n", @@ -428,9 +429,9 @@ static int ksmbd_vfs_stream_write(struct ksmbd_file *fp, char *buf, loff_t *pos, fp->stream.name, fp->stream.size, &stream_buf); - if ((int)v_len < 0) { + if (v_len < 0) { pr_err("not found stream in xattr : %zd\n", v_len); - err = (int)v_len; + err = v_len; goto out; }
The existing code works fine because ksmbd_vfs_getcasexattr() isn't going to return values greater than INT_MAX. But it's ugly to do the casting and using a ssize_t makes everything cleaner. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- fs/ksmbd/vfs.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)