* [PATCH] ksmbd: clean up a type in ksmbd_vfs_stream_write()
@ 2022-06-02 10:51 Dan Carpenter
2022-06-02 23:18 ` Namjae Jeon
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2022-06-02 10:51 UTC (permalink / raw)
To: Namjae Jeon
Cc: Steve French, Hyunchul Lee, Sergey Senozhatsky, linux-cifs,
kernel-janitors
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ksmbd: clean up a type in ksmbd_vfs_stream_write()
2022-06-02 10:51 [PATCH] ksmbd: clean up a type in ksmbd_vfs_stream_write() Dan Carpenter
@ 2022-06-02 23:18 ` Namjae Jeon
2022-06-03 0:10 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Namjae Jeon @ 2022-06-02 23:18 UTC (permalink / raw)
To: Dan Carpenter
Cc: Steve French, Hyunchul Lee, Sergey Senozhatsky, linux-cifs,
kernel-janitors
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ksmbd: clean up a type in ksmbd_vfs_stream_write()
2022-06-02 23:18 ` Namjae Jeon
@ 2022-06-03 0:10 ` Dan Carpenter
2022-06-03 0:45 ` Namjae Jeon
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2022-06-03 0:10 UTC (permalink / raw)
To: Namjae Jeon
Cc: Steve French, Hyunchul Lee, Sergey Senozhatsky, linux-cifs,
kernel-janitors
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ksmbd: clean up a type in ksmbd_vfs_stream_write()
2022-06-03 0:10 ` Dan Carpenter
@ 2022-06-03 0:45 ` Namjae Jeon
2022-06-03 7:36 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Namjae Jeon @ 2022-06-03 0:45 UTC (permalink / raw)
To: Dan Carpenter
Cc: Steve French, Hyunchul Lee, Sergey Senozhatsky, linux-cifs,
kernel-janitors
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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ksmbd: clean up a type in ksmbd_vfs_stream_write()
2022-06-03 0:45 ` Namjae Jeon
@ 2022-06-03 7:36 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2022-06-03 7:36 UTC (permalink / raw)
To: Namjae Jeon
Cc: Steve French, Hyunchul Lee, Sergey Senozhatsky, linux-cifs,
kernel-janitors
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-06-03 7:37 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-02 10:51 [PATCH] ksmbd: clean up a type in ksmbd_vfs_stream_write() Dan Carpenter
2022-06-02 23:18 ` Namjae Jeon
2022-06-03 0:10 ` Dan Carpenter
2022-06-03 0:45 ` Namjae Jeon
2022-06-03 7:36 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.