* [bug report] cifsd: add file operations
@ 2021-08-31 11:43 Dan Carpenter
2021-08-31 13:17 ` Namjae Jeon
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2021-08-31 11:43 UTC (permalink / raw)
To: namjae.jeon; +Cc: linux-fsdevel
Hello Namjae Jeon,
The patch f44158485826: "cifsd: add file operations" from Mar 16,
2021, leads to the following
Smatch static checker warning:
fs/xattr.c:524 vfs_removexattr()
warn: sleeping in atomic context
fs/xattr.c
514
515 int
516 vfs_removexattr(struct user_namespace *mnt_userns, struct dentry *dentry,
517 const char *name)
518 {
519 struct inode *inode = dentry->d_inode;
520 struct inode *delegated_inode = NULL;
521 int error;
522
523 retry_deleg:
--> 524 inode_lock(inode);
525 error = __vfs_removexattr_locked(mnt_userns, dentry,
526 name, &delegated_inode);
527 inode_unlock(inode);
528
529 if (delegated_inode) {
530 error = break_deleg_wait(&delegated_inode);
531 if (!error)
532 goto retry_deleg;
533 }
534
535 return error;
536 }
The call tree is (slight edited).
ksmbd_file_table_flush() <- disables preempt
-> ksmbd_vfs_fsync()
-> ksmbd_fd_put()
-> __put_fd_final()
-> __ksmbd_close_fd()
-> __ksmbd_inode_close()
-> ksmbd_vfs_remove_xattr()
-> vfs_removexattr()
fs/ksmbd/vfs_cache.c
669 int ksmbd_file_table_flush(struct ksmbd_work *work)
670 {
671 struct ksmbd_file *fp = NULL;
672 unsigned int id;
673 int ret;
674
675 read_lock(&work->sess->file_table.lock);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Disables preemption.
676 idr_for_each_entry(work->sess->file_table.idr, fp, id) {
677 ret = ksmbd_vfs_fsync(work, fp->volatile_id, KSMBD_NO_FID);
678 if (ret)
679 break;
680 }
681 read_unlock(&work->sess->file_table.lock);
682 return ret;
683 }
Hopefully this bug report is clear why Smatch is complaining. Let me
know if you have any questions.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] cifsd: add file operations
2021-08-31 11:43 [bug report] cifsd: add file operations Dan Carpenter
@ 2021-08-31 13:17 ` Namjae Jeon
0 siblings, 0 replies; 4+ messages in thread
From: Namjae Jeon @ 2021-08-31 13:17 UTC (permalink / raw)
To: Dan Carpenter; +Cc: namjae.jeon, linux-fsdevel
2021-08-31 20:43 GMT+09:00, Dan Carpenter <dan.carpenter@oracle.com>:
> Hello Namjae Jeon,
Hi Dan,
I will fix it.
Thanks for your report!
>
> The patch f44158485826: "cifsd: add file operations" from Mar 16,
> 2021, leads to the following
> Smatch static checker warning:
>
> fs/xattr.c:524 vfs_removexattr()
> warn: sleeping in atomic context
>
> fs/xattr.c
> 514
> 515 int
> 516 vfs_removexattr(struct user_namespace *mnt_userns, struct dentry
> *dentry,
> 517 const char *name)
> 518 {
> 519 struct inode *inode = dentry->d_inode;
> 520 struct inode *delegated_inode = NULL;
> 521 int error;
> 522
> 523 retry_deleg:
> --> 524 inode_lock(inode);
> 525 error = __vfs_removexattr_locked(mnt_userns, dentry,
> 526 name, &delegated_inode);
> 527 inode_unlock(inode);
> 528
> 529 if (delegated_inode) {
> 530 error = break_deleg_wait(&delegated_inode);
> 531 if (!error)
> 532 goto retry_deleg;
> 533 }
> 534
> 535 return error;
> 536 }
>
> The call tree is (slight edited).
>
> ksmbd_file_table_flush() <- disables preempt
> -> ksmbd_vfs_fsync()
> -> ksmbd_fd_put()
> -> __put_fd_final()
> -> __ksmbd_close_fd()
> -> __ksmbd_inode_close()
> -> ksmbd_vfs_remove_xattr()
> -> vfs_removexattr()
>
> fs/ksmbd/vfs_cache.c
> 669 int ksmbd_file_table_flush(struct ksmbd_work *work)
> 670 {
> 671 struct ksmbd_file *fp = NULL;
> 672 unsigned int id;
> 673 int ret;
> 674
> 675 read_lock(&work->sess->file_table.lock);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Disables preemption.
>
> 676 idr_for_each_entry(work->sess->file_table.idr, fp, id) {
> 677 ret = ksmbd_vfs_fsync(work, fp->volatile_id,
> KSMBD_NO_FID);
> 678 if (ret)
> 679 break;
> 680 }
> 681 read_unlock(&work->sess->file_table.lock);
> 682 return ret;
> 683 }
>
> Hopefully this bug report is clear why Smatch is complaining. Let me
> know if you have any questions.
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [bug report] cifsd: add file operations
@ 2023-03-03 14:10 Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2023-03-03 14:10 UTC (permalink / raw)
To: namjae.jeon; +Cc: linux-cifs, cip-dev
Hello Namjae Jeon,
The patch f44158485826: "cifsd: add file operations" from Mar 16,
2021, leads to the following Smatch static checker warning:
fs/ksmbd/vfs.c:1040 ksmbd_vfs_fqar_lseek() warn: no lower bound on 'length'
fs/ksmbd/vfs.c:1041 ksmbd_vfs_fqar_lseek() warn: no lower bound on 'start'
fs/ksmbd/vfs.c
1021 int ksmbd_vfs_fqar_lseek(struct ksmbd_file *fp, loff_t start, loff_t length,
1022 struct file_allocated_range_buffer *ranges,
1023 unsigned int in_count, unsigned int *out_count)
1024 {
1025 struct file *f = fp->filp;
1026 struct inode *inode = file_inode(fp->filp);
1027 loff_t maxbytes = (u64)inode->i_sb->s_maxbytes, end;
1028 loff_t extent_start, extent_end;
1029 int ret = 0;
1030
1031 if (start > maxbytes)
^^^^^^^^^^^^^^^^
loff_t is signed long long. This comes from the user via
fsctl_query_allocated_ranges(). Both start and length can be negative.
So we can end up search through negative offsets. Possibly
vfs_llseek() protects us.
1032 return -EFBIG;
1033
1034 if (!in_count)
1035 return 0;
1036
1037 /*
1038 * Shrink request scope to what the fs can actually handle.
1039 */
--> 1040 if (length > maxbytes || (maxbytes - length) < start)
1041 length = maxbytes - start;
1042
1043 if (start + length > inode->i_size)
1044 length = inode->i_size - start;
1045
1046 *out_count = 0;
1047 end = start + length;
1048 while (start < end && *out_count < in_count) {
1049 extent_start = vfs_llseek(f, start, SEEK_DATA);
1050 if (extent_start < 0) {
1051 if (extent_start != -ENXIO)
1052 ret = (int)extent_start;
1053 break;
1054 }
1055
1056 if (extent_start >= end)
1057 break;
1058
1059 extent_end = vfs_llseek(f, extent_start, SEEK_HOLE);
1060 if (extent_end < 0) {
1061 if (extent_end != -ENXIO)
1062 ret = (int)extent_end;
1063 break;
1064 } else if (extent_start >= extent_end) {
1065 break;
1066 }
1067
1068 ranges[*out_count].file_offset = cpu_to_le64(extent_start);
1069 ranges[(*out_count)++].length =
1070 cpu_to_le64(min(extent_end, end) - extent_start);
1071
1072 start = extent_end;
1073 }
1074
1075 return ret;
1076 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* [bug report] cifsd: add file operations
@ 2021-07-07 10:11 Dan Carpenter
0 siblings, 0 replies; 4+ messages in thread
From: Dan Carpenter @ 2021-07-07 10:11 UTC (permalink / raw)
To: namjae.jeon; +Cc: linux-cifs
Hello Namjae Jeon,
The patch f44158485826: "cifsd: add file operations" from Mar 16,
2021, leads to the following static checker warning:
fs/ksmbd/vfs_cache.c:661 ksmbd_file_table_flush()
warn: was expecting a 64 bit value instead of '0'
fs/ksmbd/vfs_cache.c
653 int ksmbd_file_table_flush(struct ksmbd_work *work)
654 {
655 struct ksmbd_file *fp = NULL;
656 unsigned int id;
657 int ret;
658
659 read_lock(&work->sess->file_table.lock);
660 idr_for_each_entry(work->sess->file_table.idr, fp, id) {
661 ret = ksmbd_vfs_fsync(work, fp->volatile_id, KSMBD_NO_FID);
TBH, I'm not sure what's triggering this warning, but ksmbd_vfs_fsync()
takes a u64 and fp->volatile_id is a u32. The ksmbd_lookup_fd_slow()
function takes a u32 so passing a u64 value seems like it would lead to
a bug. And smb2_flush() passes a u64 so this seems buggy.
662 if (ret)
663 break;
664 }
665 read_unlock(&work->sess->file_table.lock);
666 return ret;
667 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-03-03 14:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-31 11:43 [bug report] cifsd: add file operations Dan Carpenter
2021-08-31 13:17 ` Namjae Jeon
-- strict thread matches above, loose matches on Subject: below --
2023-03-03 14:10 Dan Carpenter
2021-07-07 10:11 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.