| Message ID | 20241206164155.3c80d28e.ddiss@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ksmbd: v6.13-rc1 WARNING at fs/attr.c:300 setattr_copy+0x1ee/0x200 | expand |
On Fri, Dec 6, 2024 at 2:42 PM David Disseldorp <ddiss@suse.de> wrote: > > Hi Namjae, Hi David, > > I'm hitting the following warning while running xfstests against a > v6.13-rc1 ksmbd server: Thanks for your report:) > > [ 113.215316] ------------[ cut here ]------------ > [ 113.215974] WARNING: CPU: 1 PID: 31 at fs/attr.c:300 setattr_copy+0x1ee/0x200 > [ 113.216988] Modules linked in: btrfs blake2b_generic xor zlib_deflate raid6_pq zstd_decompress zstd_compress xxhash zstd_common ksmbd crc32_generic cifs_arc4 nls_ucs2_utis > [ 113.219192] CPU: 1 UID: 0 PID: 31 Comm: kworker/1:1 Not tainted 6.13.0-rc1+ #234 > [ 113.220127] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-3-gd478f380-rebuilt.opensuse.org 04/01/2014 > [ 113.221530] Workqueue: ksmbd-io handle_ksmbd_work [ksmbd] > [ 113.222220] RIP: 0010:setattr_copy+0x1ee/0x200 > [ 113.222833] Code: 24 28 49 8b 44 24 30 48 89 53 58 89 43 6c 5b 41 5c 41 5d 41 5e 41 5f 5d c3 cc cc cc cc 48 89 df e8 77 d6 ff ff e9 cd fe ff ff <0f> 0b e9 be fe ff ff 66 0 > [ 113.225110] RSP: 0018:ffffaf218010fb68 EFLAGS: 00010202 > [ 113.225765] RAX: 0000000000000120 RBX: ffffa446815f8568 RCX: 0000000000000003 > [ 113.226667] RDX: ffffaf218010fd38 RSI: ffffa446815f8568 RDI: ffffffff94eb03a0 > [ 113.227531] RBP: ffffaf218010fb90 R08: 0000001a251e217d R09: 00000000675259fa > [ 113.228426] R10: 0000000002ba8a6d R11: ffffa4468196c7a8 R12: ffffaf218010fd38 > [ 113.229304] R13: 0000000000000120 R14: ffffffff94eb03a0 R15: 0000000000000000 > [ 113.230210] FS: 0000000000000000(0000) GS:ffffa44739d00000(0000) knlGS:0000000000000000 > [ 113.231215] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 113.232055] CR2: 00007efe0053d27e CR3: 000000000331a000 CR4: 00000000000006b0 > [ 113.232926] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 > [ 113.233812] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 > [ 113.234797] Call Trace: > [ 113.235116] <TASK> > [ 113.235393] ? __warn+0x73/0xd0 > [ 113.235802] ? setattr_copy+0x1ee/0x200 > [ 113.236299] ? report_bug+0xf3/0x1e0 > [ 113.236757] ? handle_bug+0x4d/0x90 > [ 113.237202] ? exc_invalid_op+0x13/0x60 > [ 113.237689] ? asm_exc_invalid_op+0x16/0x20 > [ 113.238185] ? setattr_copy+0x1ee/0x200 > [ 113.238692] btrfs_setattr+0x80/0x820 [btrfs] > [ 113.239285] ? get_stack_info_noinstr+0x12/0xf0 > [ 113.239857] ? __module_address+0x22/0xa0 > [ 113.240368] ? handle_ksmbd_work+0x6e/0x460 [ksmbd] > [ 113.240993] ? __module_text_address+0x9/0x50 > [ 113.241545] ? __module_address+0x22/0xa0 > [ 113.242033] ? unwind_next_frame+0x10e/0x920 > [ 113.242600] ? __pfx_stack_trace_consume_entry+0x10/0x10 > [ 113.243268] notify_change+0x2c2/0x4e0 > [ 113.243746] ? stack_depot_save_flags+0x27/0x730 > [ 113.244339] ? set_file_basic_info+0x130/0x2b0 [ksmbd] > [ 113.244993] set_file_basic_info+0x130/0x2b0 [ksmbd] > [ 113.245613] ? process_scheduled_works+0xbe/0x310 > [ 113.246181] ? worker_thread+0x100/0x240 > [ 113.246696] ? kthread+0xc8/0x100 > [ 113.247126] ? ret_from_fork+0x2b/0x40 > [ 113.247606] ? ret_from_fork_asm+0x1a/0x30 > [ 113.248132] smb2_set_info+0x63f/0xa70 [ksmbd] > > > 284 static void setattr_copy_mgtime(struct inode *inode, const struct iattr *attr) > 285 { > 286 unsigned int ia_valid = attr->ia_valid; > 287 struct timespec64 now; > 288 > 289 if (ia_valid & ATTR_CTIME) { > 290 /* > 291 * In the case of an update for a write delegation, we must respect > 292 * the value in ia_ctime and not use the current time. > 293 */ > 294 if (ia_valid & ATTR_DELEG) > 295 now = inode_set_ctime_deleg(inode, attr->ia_ctime); > 296 else > 297 now = inode_set_ctime_current(inode); > 298 } else { > 299 /* If ATTR_CTIME isn't set, then ATTR_MTIME shouldn't be either. */ > 300 WARN_ON_ONCE(ia_valid & ATTR_MTIME); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^--- here. > > Looking at smb2pdu.c:set_file_basic_info() it's easy enough to see how > we can get here with !ATTR_CTIME alongside ATTR_MTIME. > > The following patch avoids the warning, but I'm not familiar with this > code-path, so please let me know whether or not it makes sense: mtime and atime will probably not be updated. I will change it so that ATTR_CTIME is also set when changing mtime. Thanks. > > --- a/fs/smb/server/smb2pdu.c > +++ b/fs/smb/server/smb2pdu.c > @@ -6013,7 +6013,7 @@ static int set_file_basic_info(struct ksmbd_file *fp, > > if (file_info->LastAccessTime) { > attrs.ia_atime = ksmbd_NTtimeToUnix(file_info->LastAccessTime); > - attrs.ia_valid |= (ATTR_ATIME | ATTR_ATIME_SET); > + attrs.ia_valid |= ATTR_ATIME_SET; > } > > attrs.ia_valid |= ATTR_CTIME; > @@ -6024,7 +6024,7 @@ static int set_file_basic_info(struct ksmbd_file *fp, > > if (file_info->LastWriteTime) { > attrs.ia_mtime = ksmbd_NTtimeToUnix(file_info->LastWriteTime); > - attrs.ia_valid |= (ATTR_MTIME | ATTR_MTIME_SET); > + attrs.ia_valid |= ATTR_MTIME_SET; > } > > if (file_info->Attributes) {
On Fri, 6 Dec 2024 16:35:18 +0900, Namjae Jeon wrote: ... > > 300 WARN_ON_ONCE(ia_valid & ATTR_MTIME); > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^--- here. I should mention, fstests generic/215 atop a 6.13.0-rc1 cifs.ko SMB3 mount was the trigger for this ksmbd warning. > > Looking at smb2pdu.c:set_file_basic_info() it's easy enough to see how > > we can get here with !ATTR_CTIME alongside ATTR_MTIME. > > > > The following patch avoids the warning, but I'm not familiar with this > > code-path, so please let me know whether or not it makes sense: > mtime and atime will probably not be updated. Unless I'm missing something, this patched ksmbd still triggers mtime update via the setattr_copy_mgtime()->(ia_valid & ATTR_MTIME_SET) path. > I will change it so that ATTR_CTIME is also set when changing mtime. That should also work. I was turned off that path due to the 64e7875560270 ("ksmbd: fix oops from fuse driver") ATTR_CTIME changes. Thanks, David
On Fri, Dec 6, 2024 at 9:37 PM David Disseldorp <ddiss@suse.de> wrote: > > On Fri, 6 Dec 2024 16:35:18 +0900, Namjae Jeon wrote: > ... > > > 300 WARN_ON_ONCE(ia_valid & ATTR_MTIME); > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^--- here. > > I should mention, fstests generic/215 atop a 6.13.0-rc1 cifs.ko SMB3 > mount was the trigger for this ksmbd warning. Okay, I have reproduced it. Thanks. I will take a look. > > > > Looking at smb2pdu.c:set_file_basic_info() it's easy enough to see how > > > we can get here with !ATTR_CTIME alongside ATTR_MTIME. > > > > > > The following patch avoids the warning, but I'm not familiar with this > > > code-path, so please let me know whether or not it makes sense: > > mtime and atime will probably not be updated. > > Unless I'm missing something, this patched ksmbd still triggers mtime > update via the setattr_copy_mgtime()->(ia_valid & ATTR_MTIME_SET) path. If FS_MGTIME flag is not set in the fstype of local filesystem, mtime is not updated with only ATTR_MTIME_SET. Thanks. > > > I will change it so that ATTR_CTIME is also set when changing mtime. > > That should also work. I was turned off that path due to the > 64e7875560270 ("ksmbd: fix oops from fuse driver") ATTR_CTIME changes. > > Thanks, David
--- a/fs/smb/server/smb2pdu.c +++ b/fs/smb/server/smb2pdu.c @@ -6013,7 +6013,7 @@ static int set_file_basic_info(struct ksmbd_file *fp, if (file_info->LastAccessTime) { attrs.ia_atime = ksmbd_NTtimeToUnix(file_info->LastAccessTime); - attrs.ia_valid |= (ATTR_ATIME | ATTR_ATIME_SET); + attrs.ia_valid |= ATTR_ATIME_SET; } attrs.ia_valid |= ATTR_CTIME; @@ -6024,7 +6024,7 @@ static int set_file_basic_info(struct ksmbd_file *fp, if (file_info->LastWriteTime) { attrs.ia_mtime = ksmbd_NTtimeToUnix(file_info->LastWriteTime); - attrs.ia_valid |= (ATTR_MTIME | ATTR_MTIME_SET); + attrs.ia_valid |= ATTR_MTIME_SET; } if (file_info->Attributes) {
Hi Namjae, I'm hitting the following warning while running xfstests against a v6.13-rc1 ksmbd server: [ 113.215316] ------------[ cut here ]------------ [ 113.215974] WARNING: CPU: 1 PID: 31 at fs/attr.c:300 setattr_copy+0x1ee/0x200 [ 113.216988] Modules linked in: btrfs blake2b_generic xor zlib_deflate raid6_pq zstd_decompress zstd_compress xxhash zstd_common ksmbd crc32_generic cifs_arc4 nls_ucs2_utis [ 113.219192] CPU: 1 UID: 0 PID: 31 Comm: kworker/1:1 Not tainted 6.13.0-rc1+ #234 [ 113.220127] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.2-3-gd478f380-rebuilt.opensuse.org 04/01/2014 [ 113.221530] Workqueue: ksmbd-io handle_ksmbd_work [ksmbd] [ 113.222220] RIP: 0010:setattr_copy+0x1ee/0x200 [ 113.222833] Code: 24 28 49 8b 44 24 30 48 89 53 58 89 43 6c 5b 41 5c 41 5d 41 5e 41 5f 5d c3 cc cc cc cc 48 89 df e8 77 d6 ff ff e9 cd fe ff ff <0f> 0b e9 be fe ff ff 66 0 [ 113.225110] RSP: 0018:ffffaf218010fb68 EFLAGS: 00010202 [ 113.225765] RAX: 0000000000000120 RBX: ffffa446815f8568 RCX: 0000000000000003 [ 113.226667] RDX: ffffaf218010fd38 RSI: ffffa446815f8568 RDI: ffffffff94eb03a0 [ 113.227531] RBP: ffffaf218010fb90 R08: 0000001a251e217d R09: 00000000675259fa [ 113.228426] R10: 0000000002ba8a6d R11: ffffa4468196c7a8 R12: ffffaf218010fd38 [ 113.229304] R13: 0000000000000120 R14: ffffffff94eb03a0 R15: 0000000000000000 [ 113.230210] FS: 0000000000000000(0000) GS:ffffa44739d00000(0000) knlGS:0000000000000000 [ 113.231215] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 113.232055] CR2: 00007efe0053d27e CR3: 000000000331a000 CR4: 00000000000006b0 [ 113.232926] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 113.233812] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 [ 113.234797] Call Trace: [ 113.235116] <TASK> [ 113.235393] ? __warn+0x73/0xd0 [ 113.235802] ? setattr_copy+0x1ee/0x200 [ 113.236299] ? report_bug+0xf3/0x1e0 [ 113.236757] ? handle_bug+0x4d/0x90 [ 113.237202] ? exc_invalid_op+0x13/0x60 [ 113.237689] ? asm_exc_invalid_op+0x16/0x20 [ 113.238185] ? setattr_copy+0x1ee/0x200 [ 113.238692] btrfs_setattr+0x80/0x820 [btrfs] [ 113.239285] ? get_stack_info_noinstr+0x12/0xf0 [ 113.239857] ? __module_address+0x22/0xa0 [ 113.240368] ? handle_ksmbd_work+0x6e/0x460 [ksmbd] [ 113.240993] ? __module_text_address+0x9/0x50 [ 113.241545] ? __module_address+0x22/0xa0 [ 113.242033] ? unwind_next_frame+0x10e/0x920 [ 113.242600] ? __pfx_stack_trace_consume_entry+0x10/0x10 [ 113.243268] notify_change+0x2c2/0x4e0 [ 113.243746] ? stack_depot_save_flags+0x27/0x730 [ 113.244339] ? set_file_basic_info+0x130/0x2b0 [ksmbd] [ 113.244993] set_file_basic_info+0x130/0x2b0 [ksmbd] [ 113.245613] ? process_scheduled_works+0xbe/0x310 [ 113.246181] ? worker_thread+0x100/0x240 [ 113.246696] ? kthread+0xc8/0x100 [ 113.247126] ? ret_from_fork+0x2b/0x40 [ 113.247606] ? ret_from_fork_asm+0x1a/0x30 [ 113.248132] smb2_set_info+0x63f/0xa70 [ksmbd] 284 static void setattr_copy_mgtime(struct inode *inode, const struct iattr *attr) 285 { 286 unsigned int ia_valid = attr->ia_valid; 287 struct timespec64 now; 288 289 if (ia_valid & ATTR_CTIME) { 290 /* 291 * In the case of an update for a write delegation, we must respect 292 * the value in ia_ctime and not use the current time. 293 */ 294 if (ia_valid & ATTR_DELEG) 295 now = inode_set_ctime_deleg(inode, attr->ia_ctime); 296 else 297 now = inode_set_ctime_current(inode); 298 } else { 299 /* If ATTR_CTIME isn't set, then ATTR_MTIME shouldn't be either. */ 300 WARN_ON_ONCE(ia_valid & ATTR_MTIME); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^--- here. Looking at smb2pdu.c:set_file_basic_info() it's easy enough to see how we can get here with !ATTR_CTIME alongside ATTR_MTIME. The following patch avoids the warning, but I'm not familiar with this code-path, so please let me know whether or not it makes sense: