[PATCH] ksmbd: clean up a type in ksmbd_vfs_stream_write()

[PATCH] ksmbd: clean up a type in ksmbd_vfs_stream_write()

[Dan Carpenter]

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;
 		goto out;
 	}
 
-- 
2.35.1

Re: [PATCH] ksmbd: clean up a type in ksmbd_vfs_stream_write()

[Namjae Jeon]

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
>
>

Re: [PATCH] ksmbd: clean up a type in ksmbd_vfs_stream_write()

[Dan Carpenter]

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

Re: [PATCH] ksmbd: clean up a type in ksmbd_vfs_stream_write()

[Namjae Jeon]

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
>
>

Re: [PATCH] ksmbd: clean up a type in ksmbd_vfs_stream_write()

[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