[Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[Salvatore Bonaccorso]

Hi

Looping in as well regressions@lists.linux.dev list (and add
linux-cifs@vger.kernel.org as well here).

On Wed, Jan 03, 2024 at 11:40:04PM +0000, Jitindar Singh, Suraj wrote:
> Hi,
> 
> When testing the v6.1.69 kernel I bisected an issue to the below commit
> which was added in v6.1.68. When running the xfstests[1] on cifs I
> observe a null pointer dereference in cifs_flush_folio() because folio
> is null and dereferenced in size = folio_size(folio).
> 
> [1] https://github.com/kdave/xfstests
> 
> This can be reproduced using many of the xfstests but reproduces with
> generic/001 like below:
> 
> $ ./check -cifs generic/001
> 
> FSTYP         -- cifs
> PLATFORM      -- Linux/x86_64 ip-172-31-36-150 6.1.69 #2 SMP
> PREEMPT_DYNAMIC Wed Jan  3 22:36:43 UTC 2024
> MKFS_OPTIONS  -- //172.31.34.66/scratch
> MOUNT_OPTIONS -- -ousername=ec2-
> user,password=abc123,noperm,mfsymlinks,cifsacl,actimeo=0 -o
> context=system_u:object_r:root_t:s0 //172.31.34.66/scratch /mnt/scratch
> 
> generic/001 10s ...  
> 
> [  244.135555] run fstests generic/001 at 2024-01-03 22:44:59
> [  244.637499] BUG: kernel NULL pointer dereference, address:
> 0000000000000000
> [  244.638204] #PF: supervisor read access in kernel mode
> [  244.638698] #PF: error_code(0x0000) - not-present page
> [  244.639194] PGD 0 P4D 0 
> [  244.639466] Oops: 0000 [#1] PREEMPT SMP NOPTI
> [  244.698047] CPU: 11 PID: 4123 Comm: cp Not tainted 6.1.69 #2
> [  244.698598] Hardware name: Amazon EC2 m6i.4xlarge/, BIOS 1.0
> 10/16/2017
> [  244.699228] RIP: 0010:cifs_flush_folio+0x3f/0x100 [cifs]
> [  244.699782] Code: d2 41 54 89 cc 31 c9 55 53 48 89 f3 48 c1 ee 0c 48
> 83 ec 08 48 8b 7f 30 e8 7d 2c a6 f8 48 3d 00 f0 ff ff 0f 87 a5 00 00 00
> <48> 8b 10 48 89 c5 b8 00 10 00 00 f7 c2 00 00 01 00 74 07 0f b6 4d
> [  244.799263] RSP: 0018:ffffc25441ffbd20 EFLAGS: 00010207
> [  244.888911] RAX: 0000000000000000 RBX: 0000000000000000 RCX:
> 0000000000000000
> [  244.898446] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
> ffff9ed945eac000
> [  244.899136] RBP: 00000000000003a1 R08: 0000000000000001 R09:
> 0000000000000000
> [  244.997779] R10: 0000000000003e7f R11: 0000000000000000 R12:
> ffffc25441ffbd90
> [  244.998564] R13: ffffc25441ffbd88 R14: ffff9ed94af23850 R15:
> 0000000000000001
> [  244.999264] FS:  00007f111404d340(0000) GS:ffff9ee7fecc0000(0000)
> knlGS:0000000000000000
> [  245.097782] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  245.098345] CR2: 0000000000000000 CR3: 00000001211fc001 CR4:
> 00000000007706e0
> [  245.099122] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000
> [  245.099854] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> 0000000000000400
> [  245.198200] PKRU: 55555554
> [  245.198478] Call Trace:
> [  245.198735]  <TASK>
> [  245.198967]  ? show_trace_log_lvl+0x1c4/0x2d2
> [  245.199399]  ? show_trace_log_lvl+0x1c4/0x2d2
> [  245.199830]  ? cifs_remap_file_range+0x15d/0x5e0 [cifs]
> [  245.298029]  ? __die_body.cold+0x8/0xd
> [  245.298402]  ? page_fault_oops+0xac/0x140
> [  245.298943]  ? exc_page_fault+0x62/0x140
> [  245.299336]  ? asm_exc_page_fault+0x22/0x30
> [  245.299751]  ? cifs_flush_folio+0x3f/0x100 [cifs]
> [  245.397858]  ? cifs_precopy_set_eof+0x73/0x110 [cifs]
> [  245.398383]  cifs_remap_file_range+0x15d/0x5e0 [cifs]
> [  245.398909]  do_clone_file_range+0xe7/0x230
> [  245.399329]  vfs_clone_file_range+0x37/0x140
> [  245.399861]  ioctl_file_clone+0x45/0xa0
> [  245.497918]  do_vfs_ioctl+0x79/0x890
> [  245.498328]  __x64_sys_ioctl+0x69/0xc0
> [  245.498706]  do_syscall_64+0x38/0x90
> [  245.499070]  entry_SYSCALL_64_after_hwframe+0x64/0xce
> [  245.499568] RIP: 0033:0x7f1113e3ec6b
> [  245.499929] Code: 73 01 c3 48 8b 0d 9500 f7 d8 64 89 01 48 83 c8 ff
> c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 10 00 00 00 0f 05
> <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 65 a1 1b 00 f7 d8 64 89 01 48
> [  245.599410] RSP: 002b:00007fff140ebb88 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000010
> [  245.697930] RAX: ffffffffffffffda RBX: 00007fff140ec3b0 RCX:
> 00007f1113e3ec6b
> [  245.698620] RDX: 0000000000000003 RSI: 0000000040049409 RDI:
> 0000000000000004
> [  245.699306] RBP: 0000000000000003 R08: 0000000000000001 R09:
> 0000000000000004
> [  245.797942] R10: 0000000000001000 R11: 0000000000000246 R12:
> 00007fff140ed616
> [  245.798789] R13: 00007fff140ebfa0 R14: 0000000000000000 R15:
> 0000000000000002
> [  245.799485]  </TASK>
> [  245.799720] Modules linked in: cmac nls_utf8 cifs cifs_arc4 cifs_md4
> dns_lver mousedev sunrpc nls_ascii nls_cp437 vfat fat psmouse
> ghash_clmulni_intel atkbd libps2 vivaldi_fmap aesni_intel ena i8042
> crypto_simd serio cryptd button drm sch_fq_codel fuse i2c_core configfs
> drm_panel_orientation_quirks loop backlight dmi_sysfs crc32_pclmul
> intel dm_mirror dm_region_hash dm_log dm_mod dax efivarfs
> [  245.998457] CR2: 0000000000000000
> [  245.998800] ---[ end trace 0000000000000000 ]---
> [  246.087916] RIP: 0010:cifs_flush_folio+0x3f/0x100 [cifs]
> [  246.088794] Code: d2 41 54 49 89 cc 31 c9 55 53 48 89 f3 48 c1 ee 0c
> 48 83 ec 08 48 8b 7f 30 e8 7d 2c a6 f8 48 3d 00 f0 ff ff 0f 87 a5 00 00
> 00 <48> 8b 10 48 89 c5 b8 00 10 00 00 f7 c2 00 00 01 00 74 07 0f b6 4d
> [  246.099436] RSP: 0018:ffffc25441ffbd20 EFLAGS: 00010207
> [  246.189258] RAX: 0000000000000000 RBX: 0000000000000000 RCX:
> 0000000000000000
> [  246.198724] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
> ffff9ed945eac000
> [  246.199411] RBP: 00000000000003a1 R08: 0000000000000001 R09:
> 0000000000000000
> [  246.298002] R10: 0000000000003e7f R11: 0000000000000000 R12:
> ffffc25441ffbd90
> [  246.298687] R13: ffffc25441ffbd88 R14: ffff9ed94af23850 R15:
> 0000000000000001
> [  246.299372] FS:  00007f111404d340(0000) GS:ffff9ee7fecc0000(0000)
> knlGS:0000000000000000
> [  246.398006] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  246.398569] CR2: 0000000000000000 CR3: 00000001211fc001 CR4:
> 00000000007706e0
> [  246.399254] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
> 0000000000000000
> [  246.399934] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
> 0000000000000400
> [  246.498414] PKRU: 55555554
> [  246.498698] Kernel panic - not syncing: Fatal exception
> [  246.500042] Kernel Offset: 0x38000000 from 0xffffffff81000000
> (relocation range: 0xffffffff80000000-0xffffffffbfffffff)
> [  246.698094] Rebooting in 5 seconds..
> 
> Any ideas what is causing this and what the resolution is? This doesn't
> occur on the upstream/master kernel.
> 
> Thanks,
> Suraj
> 
> On Mon, 2023-12-11 at 13:57 +0100, gregkh@linuxfoundation.org wrote:
> > 
> > This is a note to let you know that I've just added the patch titled
> > 
> >     cifs: Fix flushing, invalidation and file size with
> > copy_file_range()
> > 
> > to the 6.1-stable tree which can be found at:
> >    
> > http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
> > 
> > The filename of the patch is:
> >      cifs-fix-flushing-invalidation-and-file-size-with-
> > copy_file_range.patch
> > and it can be found in the queue-6.1 subdirectory.
> > 
> > If you, or anyone else, feels it should not be added to the stable
> > tree,
> > please let <stable@vger.kernel.org> know about it.
> > 
> > 
> > > From 7b2404a886f8b91250c31855d287e632123e1746 Mon Sep 17 00:00:00
> > > 2001
> > From: David Howells <dhowells@redhat.com>
> > Date: Fri, 1 Dec 2023 00:22:00 +0000
> > Subject: cifs: Fix flushing, invalidation and file size with
> > copy_file_range()
> > 
> > From: David Howells <dhowells@redhat.com>
> > 
> > commit 7b2404a886f8b91250c31855d287e632123e1746 upstream.
> > 
> > Fix a number of issues in the cifs filesystem implementation of the
> > copy_file_range() syscall in cifs_file_copychunk_range().
> > 
> > Firstly, the invalidation of the destination range is handled
> > incorrectly:
> > We shouldn't just invalidate the whole file as dirty data in the file
> > may
> > get lost and we can't just call truncate_inode_pages_range() to
> > invalidate
> > the destination range as that will erase parts of a partial folio at
> > each
> > end whilst invalidating and discarding all the folios in the middle. 
> > We
> > need to force all the folios covering the range to be reloaded, but
> > we
> > mustn't lose dirty data in them that's not in the destination range.
> > 
> > Further, we shouldn't simply round out the range to PAGE_SIZE at each
> > end
> > as cifs should move to support multipage folios.
> > 
> > Secondly, there's an issue whereby a write may have extended the file
> > locally, but not have been written back yet.  This can leaves the
> > local
> > idea of the EOF at a later point than the server's EOF.  If a copy
> > request
> > is issued, this will fail on the server with STATUS_INVALID_VIEW_SIZE
> > (which gets translated to -EIO locally) if the copy source extends
> > past the
> > server's EOF.
> > 
> > Fix this by:
> > 
> >  (0) Flush the source region (already done).  The flush does nothing
> > and
> >      the EOF isn't moved if the source region has no dirty data.
> > 
> >  (1) Move the EOF to the end of the source region if it isn't already
> > at
> >      least at this point.  If we can't do this, for instance if the
> > server
> >      doesn't support it, just flush the entire source file.
> > 
> >  (2) Find the folio (if present) at each end of the range, flushing
> > it and
> >      increasing the region-to-be-invalidated to cover those in their
> >      entirety.
> > 
> >  (3) Fully discard all the folios covering the range as we want them
> > to be
> >      reloaded.
> > 
> >  (4) Then perform the copy.
> > 
> > Thirdly, set i_size after doing the copychunk_range operation as this
> > value
> > may be used by various things internally.  stat() hides the issue
> > because
> > setting ->time to 0 causes cifs_getatr() to revalidate the
> > attributes.
> > 
> > These were causing the generic/075 xfstest to fail.
> > 
> > Fixes: 620d8745b35d ("Introduce cifs_copy_file_range()")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > cc: Paulo Alcantara <pc@manguebit.com>
> > cc: Shyam Prasad N <nspmangalore@gmail.com>
> > cc: Rohith Surabattula <rohiths.msft@gmail.com>
> > cc: Matthew Wilcox <willy@infradead.org>
> > cc: Jeff Layton <jlayton@kernel.org>
> > cc: linux-cifs@vger.kernel.org
> > cc: linux-mm@kvack.org
> > Signed-off-by: David Howells <dhowells@redhat.com>
> > Signed-off-by: Steve French <stfrench@microsoft.com>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  fs/smb/client/cifsfs.c |  102
> > +++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 99 insertions(+), 3 deletions(-)
> > 
> > --- a/fs/smb/client/cifsfs.c
> > +++ b/fs/smb/client/cifsfs.c
> > @@ -1191,6 +1191,72 @@ const struct inode_operations cifs_symli
> >         .listxattr = cifs_listxattr,
> >  };
> >  
> > +/*
> > + * Advance the EOF marker to after the source range.
> > + */
> > +static int cifs_precopy_set_eof(struct inode *src_inode, struct
> > cifsInodeInfo *src_cifsi,
> > +                               struct cifs_tcon *src_tcon,
> > +                               unsigned int xid, loff_t src_end)
> > +{
> > +       struct cifsFileInfo *writeable_srcfile;
> > +       int rc = -EINVAL;
> > +
> > +       writeable_srcfile = find_writable_file(src_cifsi,
> > FIND_WR_FSUID_ONLY);
> > +       if (writeable_srcfile) {
> > +               if (src_tcon->ses->server->ops->set_file_size)
> > +                       rc = src_tcon->ses->server->ops-
> > >set_file_size(
> > +                               xid, src_tcon, writeable_srcfile,
> > +                               src_inode->i_size, true /* no need to
> > set sparse */);
> > +               else
> > +                       rc = -ENOSYS;
> > +               cifsFileInfo_put(writeable_srcfile);
> > +               cifs_dbg(FYI, "SetFSize for copychunk rc = %d\n",
> > rc);
> > +       }
> > +
> > +       if (rc < 0)
> > +               goto set_failed;
> > +
> > +       netfs_resize_file(&src_cifsi->netfs, src_end);
> > +       fscache_resize_cookie(cifs_inode_cookie(src_inode), src_end);
> > +       return 0;
> > +
> > +set_failed:
> > +       return filemap_write_and_wait(src_inode->i_mapping);
> > +}
> > +
> > +/*
> > + * Flush out either the folio that overlaps the beginning of a range
> > in which
> > + * pos resides or the folio that overlaps the end of a range unless
> > that folio
> > + * is entirely within the range we're going to invalidate.  We
> > extend the flush
> > + * bounds to encompass the folio.
> > + */
> > +static int cifs_flush_folio(struct inode *inode, loff_t pos, loff_t
> > *_fstart, loff_t *_fend,
> > +                           bool first)
> > +{
> > +       struct folio *folio;
> > +       unsigned long long fpos, fend;
> > +       pgoff_t index = pos / PAGE_SIZE;
> > +       size_t size;
> > +       int rc = 0;
> > +
> > +       folio = filemap_get_folio(inode->i_mapping, index);
> > +       if (IS_ERR(folio))
> > +               return 0;
> > +
> > +       size = folio_size(folio);
> > +       fpos = folio_pos(folio);
> > +       fend = fpos + size - 1;
> > +       *_fstart = min_t(unsigned long long, *_fstart, fpos);
> > +       *_fend   = max_t(unsigned long long, *_fend, fend);
> > +       if ((first && pos == fpos) || (!first && pos == fend))
> > +               goto out;
> > +
> > +       rc = filemap_write_and_wait_range(inode->i_mapping, fpos,
> > fend);
> > +out:
> > +       folio_put(folio);
> > +       return rc;
> > +}
> > +
> >  static loff_t cifs_remap_file_range(struct file *src_file, loff_t
> > off,
> >                 struct file *dst_file, loff_t destoff, loff_t len,
> >                 unsigned int remap_flags)
> > @@ -1260,10 +1326,12 @@ ssize_t cifs_file_copychunk_range(unsign
> >  {
> >         struct inode *src_inode = file_inode(src_file);
> >         struct inode *target_inode = file_inode(dst_file);
> > +       struct cifsInodeInfo *src_cifsi = CIFS_I(src_inode);
> >         struct cifsFileInfo *smb_file_src;
> >         struct cifsFileInfo *smb_file_target;
> >         struct cifs_tcon *src_tcon;
> >         struct cifs_tcon *target_tcon;
> > +       unsigned long long destend, fstart, fend;
> >         ssize_t rc;
> >  
> >         cifs_dbg(FYI, "copychunk range\n");
> > @@ -1303,13 +1371,41 @@ ssize_t cifs_file_copychunk_range(unsign
> >         if (rc)
> >                 goto unlock;
> >  
> > -       /* should we flush first and last page first */
> > -       truncate_inode_pages(&target_inode->i_data, 0);
> > +       /* The server-side copy will fail if the source crosses the
> > EOF marker.
> > +        * Advance the EOF marker after the flush above to the end of
> > the range
> > +        * if it's short of that.
> > +        */
> > +       if (src_cifsi->server_eof < off + len) {
> > +               rc = cifs_precopy_set_eof(src_inode, src_cifsi,
> > src_tcon, xid, off + len);
> > +               if (rc < 0)
> > +                       goto unlock;
> > +       }
> > +
> > +       destend = destoff + len - 1;
> > +
> > +       /* Flush the folios at either end of the destination range to
> > prevent
> > +        * accidental loss of dirty data outside of the range.
> > +        */
> > +       fstart = destoff;
> > +       fend = destend;
> > +
> > +       rc = cifs_flush_folio(target_inode, destoff, &fstart, &fend,
> > true);
> > +       if (rc)
> > +               goto unlock;
> > +       rc = cifs_flush_folio(target_inode, destend, &fstart, &fend,
> > false);
> > +       if (rc)
> > +               goto unlock;
> > +
> > +       /* Discard all the folios that overlap the destination
> > region. */
> > +       truncate_inode_pages_range(&target_inode->i_data, fstart,
> > fend);
> >  
> >         rc = file_modified(dst_file);
> > -       if (!rc)
> > +       if (!rc) {
> >                 rc = target_tcon->ses->server->ops-
> > >copychunk_range(xid,
> >                         smb_file_src, smb_file_target, off, len,
> > destoff);
> > +               if (rc > 0 && destoff + rc >
> > i_size_read(target_inode))
> > +                       truncate_setsize(target_inode, destoff + rc);
> > +       }
> >  
> >         file_accessed(src_file);
> >  
> > 
> > 
> > Patches currently in stable-queue which might be from
> > dhowells@redhat.com are
> > 
> > queue-6.1/cifs-fix-flushing-invalidation-and-file-size-with-
> > copy_file_range.patch
> > queue-6.1/cifs-fix-flushing-invalidation-and-file-size-with-
> > ficlone.patch
> > queue-6.1/cifs-fix-non-availability-of-dedup-breaking-generic-
> > 304.patch
> > 
> 

It looks we had as well some reports in Debian about this regression.
Copying a file within the same cifs mount seems to trigger the NULL
pointer dereference:

[ 1640.742300] BUG: kernel NULL pointer dereference, address: 0000000000000000
[ 1640.742309] #PF: supervisor read access in kernel mode
[ 1640.742313] #PF: error_code(0x0000) - not-present page
[ 1640.742317] PGD 0 P4D 0
[ 1640.742322] Oops: 0000 [#1] PREEMPT SMP NOPTI
[ 1640.742327] CPU: 6 PID: 1972 Comm: cp Not tainted 6.1.0-17-amd64 #1  Debian 6.1.69-1
[ 1640.742333] Hardware name: LENOVO 20XXS1FU00/20XXS1FU00, BIOS N32ET89W (1.65 ) 11/13/2023
[ 1640.742336] RIP: 0010:cifs_flush_folio+0x3f/0x100 [cifs]
[ 1640.742412] Code: d2 41 54 49 89 cc 31 c9 55 48 89 f5 48 c1 ee 0c 53 48 83 ec 08 48 8b 7f 30 e8 8d 9a 97 dd 48 3d 00 f0 ff ff 0f 87 a5 00 00 00 <48> 8b 10 48 89 c3 b8 00 10 00 00 f7 c2 00 00 01 00 74 07 0f b6 4b
[ 1640.742417] RSP: 0018:ffff9eac8181fd18 EFLAGS: 00010207
[ 1640.742422] RAX: 0000000000000000 RBX: 0000000000000004 RCX: 0000000000000000
[ 1640.742425] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff8a0a758b0000
[ 1640.742428] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
[ 1640.742431] R10: 0000000000000003 R11: ffff8a09c596fa00 R12: ffff9eac8181fd88
[ 1640.742433] R13: ffff9eac8181fd80 R14: ffff8a0a707bda48 R15: 0000000000000001
[ 1640.742437] FS:  00007f7dc8764800(0000) GS:ffff8a10ff780000(0000) knlGS:0000000000000000
[ 1640.742441] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1640.742444] CR2: 0000000000000000 CR3: 000000015eff0003 CR4: 0000000000770ee0
[ 1640.742448] PKRU: 55555554
[ 1640.742450] Call Trace:
[ 1640.742454]  <TASK>
[ 1640.742459]  ? __die_body.cold+0x1a/0x1f
[ 1640.742468]  ? page_fault_oops+0xd2/0x2b0
[ 1640.742476]  ? exc_page_fault+0x70/0x170
[ 1640.742482]  ? asm_exc_page_fault+0x22/0x30
[ 1640.742492]  ? cifs_flush_folio+0x3f/0x100 [cifs]
[ 1640.742558]  ? cifs_flush_folio+0x33/0x100 [cifs]
[ 1640.742622]  ? cifs_precopy_set_eof+0x2b/0x150 [cifs]
[ 1640.742688]  cifs_remap_file_range+0x16d/0x680 [cifs]
[ 1640.742755]  do_clone_file_range+0xe6/0x230
[ 1640.742762]  vfs_clone_file_range+0x37/0x140
[ 1640.742767]  ioctl_file_clone+0x49/0xb0
[ 1640.742774]  do_vfs_ioctl+0x77/0x910
[ 1640.742780]  __x64_sys_ioctl+0x6e/0xd0
[ 1640.742785]  do_syscall_64+0x58/0xc0
[ 1640.742794]  entry_SYSCALL_64_after_hwframe+0x64/0xce
[ 1640.742801] RIP: 0033:0x7f7dc8900b5b
[ 1640.742806] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1c 48 8b 44 24 18 64 48 2b 04 25 28 00 00
[ 1640.742810] RSP: 002b:00007ffe4b899000 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[ 1640.742815] RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007f7dc8900b5b
[ 1640.742817] RDX: 0000000000000003 RSI: 0000000040049409 RDI: 0000000000000004
[ 1640.742820] RBP: 00007ffe4b899440 R08: 00007ffe4b899600 R09: 0000000000000001
[ 1640.742823] R10: 00007f7dc881a358 R11: 0000000000000246 R12: 0000000000000001
[ 1640.742825] R13: 00007ffe4b899dc3 R14: 0000000000008000 R15: 0000000000000000
[ 1640.742831]  </TASK>
[ 1640.742832] Modules linked in: nls_utf8 cifs cifs_arc4 cifs_md4 dns_resolver fscache netfs ctr ccm rfcomm cmac algif_hash algif_skcipher af_alg snd_seq_dummy snd_hrtimer snd_seq snd_seq_device bnep btusb btrtl btbcm btintel btmtk bluetooth uvcvideo videobuf2_vmalloc videobuf2_memops jitterentropy_rng videobuf2_v4l2 videobuf2_common drbg videodev ansi_cprng mc ecdh_generic ecc qrtr ip6t_REJECT nf_reject_ipv6 xt_hl ip6_tables ip6t_rt snd_ctl_led snd_soc_skl_hda_dsp snd_soc_intel_hda_dsp_common binfmt_misc snd_soc_hdac_hdmi ipt_REJECT snd_sof_probes nf_reject_ipv4 xt_LOG nf_log_syslog xt_comment nft_limit xt_limit xt_addrtype snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic snd_soc_dmic snd_sof_pci_intel_tgl snd_sof_intel_hda_common soundwire_intel soundwire_generic_allocation joydev soundwire_cadence snd_sof_intel_hda x86_pkg_temp_thermal snd_sof_pci intel_powerclamp snd_sof_xtensa_dsp coretemp snd_sof snd_sof_utils snd_soc_hdac_hda kvm_intel snd_hda_ext_core
[ 1640.742921]  snd_soc_acpi_intel_match xt_tcpudp snd_soc_acpi kvm snd_soc_core xt_conntrack irqbypass nf_conntrack crc32_pclmul snd_compress nf_defrag_ipv6 soundwire_bus nf_defrag_ipv4 ghash_clmulni_intel iwlmvm nft_compat snd_hda_intel sha512_ssse3 snd_intel_dspcfg nf_tables snd_intel_sdw_acpi sha512_generic mac80211 libcrc32c snd_hda_codec hid_multitouch nfnetlink processor_thermal_device_pci_legacy hid_generic sha256_ssse3 iTCO_wdt snd_hda_core processor_thermal_device sha1_ssse3 intel_pmc_bxt iTCO_vendor_support processor_thermal_rfim libarc4 pmt_telemetry rapl thinkpad_acpi snd_hwdep xhci_pci processor_thermal_mbox mei_hdcp watchdog intel_rapl_msr snd_pcm pmt_class nls_ascii nvram intel_cstate processor_thermal_rapl xhci_hcd ucsi_acpi iwlwifi platform_profile snd_timer nls_cp437 intel_uncore i2c_hid_acpi intel_lpss_pci usbcore typec_ucsi ledtrig_audio think_lmi mei_me i2c_i801 intel_rapl_common snd pcspkr vfat i2c_hid intel_lpss roles cfg80211 firmware_attributes_class mei
[ 1640.743007]  thunderbolt usb_common soundcore wmi_bmof fat int3403_thermal idma64 i2c_smbus intel_vsec typec intel_soc_dts_iosf hid igen6_edac button battery ac rfkill soc_button_array int340x_thermal_zone int3400_thermal intel_hid intel_pmc_core acpi_thermal_rel acpi_tad acpi_pad sparse_keymap msr parport_pc ppdev lp parport loop fuse efi_pstore configfs efivarfs ip_tables x_tables autofs4 ext4 crc16 mbcache jbd2 crc32c_generic dm_crypt dm_mod i915 i2c_algo_bit drm_buddy nvme drm_display_helper nvme_core crc32c_intel t10_pi drm_kms_helper crc64_rocksoft cec crc64 rc_core crc_t10dif ttm crct10dif_generic aesni_intel crct10dif_pclmul psmouse drm evdev crypto_simd cryptd crct10dif_common serio_raw video wmi
[ 1640.743095] CR2: 0000000000000000
[ 1640.743098] ---[ end trace 0000000000000000 ]---
[ 1640.957607] RIP: 0010:cifs_flush_folio+0x3f/0x100 [cifs]
[ 1640.957681] Code: d2 41 54 49 89 cc 31 c9 55 48 89 f5 48 c1 ee 0c 53 48 83 ec 08 48 8b 7f 30 e8 8d 9a 97 dd 48 3d 00 f0 ff ff 0f 87 a5 00 00 00 <48> 8b 10 48 89 c3 b8 00 10 00 00 f7 c2 00 00 01 00 74 07 0f b6 4b
[ 1640.957683] RSP: 0018:ffff9eac8181fd18 EFLAGS: 00010207
[ 1640.957685] RAX: 0000000000000000 RBX: 0000000000000004 RCX: 0000000000000000
[ 1640.957686] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff8a0a758b0000
[ 1640.957687] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
[ 1640.957688] R10: 0000000000000003 R11: ffff8a09c596fa00 R12: ffff9eac8181fd88
[ 1640.957689] R13: ffff9eac8181fd80 R14: ffff8a0a707bda48 R15: 0000000000000001
[ 1640.957691] FS:  00007f7dc8764800(0000) GS:ffff8a10ff780000(0000) knlGS:0000000000000000
[ 1640.957692] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1640.957693] CR2: 0000000000000000 CR3: 000000015eff0003 CR4: 0000000000770ee0
[ 1640.957695] PKRU: 55555554
[ 1640.957696] note: cp[1972] exited with irqs disabled

#regzbot ^introduced 18b02e4343e8f5be6a2f44c7ad9899b385a92730
#regzbot link: https://bugs.debian.org/1060005
#regzbot link: https://bugs.debian.org/1060052

Regards,
Salvatore

Re: [Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[Linux regression tracking (Thorsten Leemhuis)]

Hi, Thorsten here, the Linux kernel's regression tracker. Top-posting
for once, to make this easily accessible to everyone.

Thank's for CCIng the regression list and telling regzbot about the
issue. There is one important thing that afaics[1] is missing and would
be really good to know[2]:

Does this problem also happen in mainline, e.g. with 6.7-rc8?

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

[1] I hope I didn't miss this
[2] as explained here:
https://docs.kernel.org/admin-guide/reporting-issues.html
https://linux-regtracking.leemhuis.info/post/frequent-reasons-why-linux-kernel-bug-reports-are-ignored/#you-reported-a-regression-in-a-stable-or-longterm-series-without-checking-if-mainline-is-affected-as-well

--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

On 05.01.24 21:50, Salvatore Bonaccorso wrote:
> 
> Looping in as well regressions@lists.linux.dev list (and add
> linux-cifs@vger.kernel.org as well here).
> 
> On Wed, Jan 03, 2024 at 11:40:04PM +0000, Jitindar Singh, Suraj wrote:
>> Hi,
>>
>> When testing the v6.1.69 kernel I bisected an issue to the below commit
>> which was added in v6.1.68. When running the xfstests[1] on cifs I
>> observe a null pointer dereference in cifs_flush_folio() because folio
>> is null and dereferenced in size = folio_size(folio).
>>
>> [1] https://github.com/kdave/xfstests
>>
>> This can be reproduced using many of the xfstests but reproduces with
>> generic/001 like below:
>>
>> $ ./check -cifs generic/001
>>
>> FSTYP         -- cifs
>> PLATFORM      -- Linux/x86_64 ip-172-31-36-150 6.1.69 #2 SMP
>> PREEMPT_DYNAMIC Wed Jan  3 22:36:43 UTC 2024
>> MKFS_OPTIONS  -- //172.31.34.66/scratch
>> MOUNT_OPTIONS -- -ousername=ec2-
>> user,password=abc123,noperm,mfsymlinks,cifsacl,actimeo=0 -o
>> context=system_u:object_r:root_t:s0 //172.31.34.66/scratch /mnt/scratch
>>
>> generic/001 10s ...  
>>
>> [  244.135555] run fstests generic/001 at 2024-01-03 22:44:59
>> [  244.637499] BUG: kernel NULL pointer dereference, address:
>> 0000000000000000
>> [  244.638204] #PF: supervisor read access in kernel mode
>> [  244.638698] #PF: error_code(0x0000) - not-present page
>> [  244.639194] PGD 0 P4D 0 
>> [  244.639466] Oops: 0000 [#1] PREEMPT SMP NOPTI
>> [  244.698047] CPU: 11 PID: 4123 Comm: cp Not tainted 6.1.69 #2
>> [  244.698598] Hardware name: Amazon EC2 m6i.4xlarge/, BIOS 1.0
>> 10/16/2017
>> [  244.699228] RIP: 0010:cifs_flush_folio+0x3f/0x100 [cifs]
>> [  244.699782] Code: d2 41 54 89 cc 31 c9 55 53 48 89 f3 48 c1 ee 0c 48
>> 83 ec 08 48 8b 7f 30 e8 7d 2c a6 f8 48 3d 00 f0 ff ff 0f 87 a5 00 00 00
>> <48> 8b 10 48 89 c5 b8 00 10 00 00 f7 c2 00 00 01 00 74 07 0f b6 4d
>> [  244.799263] RSP: 0018:ffffc25441ffbd20 EFLAGS: 00010207
>> [  244.888911] RAX: 0000000000000000 RBX: 0000000000000000 RCX:
>> 0000000000000000
>> [  244.898446] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
>> ffff9ed945eac000
>> [  244.899136] RBP: 00000000000003a1 R08: 0000000000000001 R09:
>> 0000000000000000
>> [  244.997779] R10: 0000000000003e7f R11: 0000000000000000 R12:
>> ffffc25441ffbd90
>> [  244.998564] R13: ffffc25441ffbd88 R14: ffff9ed94af23850 R15:
>> 0000000000000001
>> [  244.999264] FS:  00007f111404d340(0000) GS:ffff9ee7fecc0000(0000)
>> knlGS:0000000000000000
>> [  245.097782] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [  245.098345] CR2: 0000000000000000 CR3: 00000001211fc001 CR4:
>> 00000000007706e0
>> [  245.099122] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
>> 0000000000000000
>> [  245.099854] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
>> 0000000000000400
>> [  245.198200] PKRU: 55555554
>> [  245.198478] Call Trace:
>> [  245.198735]  <TASK>
>> [  245.198967]  ? show_trace_log_lvl+0x1c4/0x2d2
>> [  245.199399]  ? show_trace_log_lvl+0x1c4/0x2d2
>> [  245.199830]  ? cifs_remap_file_range+0x15d/0x5e0 [cifs]
>> [  245.298029]  ? __die_body.cold+0x8/0xd
>> [  245.298402]  ? page_fault_oops+0xac/0x140
>> [  245.298943]  ? exc_page_fault+0x62/0x140
>> [  245.299336]  ? asm_exc_page_fault+0x22/0x30
>> [  245.299751]  ? cifs_flush_folio+0x3f/0x100 [cifs]
>> [  245.397858]  ? cifs_precopy_set_eof+0x73/0x110 [cifs]
>> [  245.398383]  cifs_remap_file_range+0x15d/0x5e0 [cifs]
>> [  245.398909]  do_clone_file_range+0xe7/0x230
>> [  245.399329]  vfs_clone_file_range+0x37/0x140
>> [  245.399861]  ioctl_file_clone+0x45/0xa0
>> [  245.497918]  do_vfs_ioctl+0x79/0x890
>> [  245.498328]  __x64_sys_ioctl+0x69/0xc0
>> [  245.498706]  do_syscall_64+0x38/0x90
>> [  245.499070]  entry_SYSCALL_64_after_hwframe+0x64/0xce
>> [  245.499568] RIP: 0033:0x7f1113e3ec6b
>> [  245.499929] Code: 73 01 c3 48 8b 0d 9500 f7 d8 64 89 01 48 83 c8 ff
>> c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa b8 10 00 00 00 0f 05
>> <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 65 a1 1b 00 f7 d8 64 89 01 48
>> [  245.599410] RSP: 002b:00007fff140ebb88 EFLAGS: 00000246 ORIG_RAX:
>> 0000000000000010
>> [  245.697930] RAX: ffffffffffffffda RBX: 00007fff140ec3b0 RCX:
>> 00007f1113e3ec6b
>> [  245.698620] RDX: 0000000000000003 RSI: 0000000040049409 RDI:
>> 0000000000000004
>> [  245.699306] RBP: 0000000000000003 R08: 0000000000000001 R09:
>> 0000000000000004
>> [  245.797942] R10: 0000000000001000 R11: 0000000000000246 R12:
>> 00007fff140ed616
>> [  245.798789] R13: 00007fff140ebfa0 R14: 0000000000000000 R15:
>> 0000000000000002
>> [  245.799485]  </TASK>
>> [  245.799720] Modules linked in: cmac nls_utf8 cifs cifs_arc4 cifs_md4
>> dns_lver mousedev sunrpc nls_ascii nls_cp437 vfat fat psmouse
>> ghash_clmulni_intel atkbd libps2 vivaldi_fmap aesni_intel ena i8042
>> crypto_simd serio cryptd button drm sch_fq_codel fuse i2c_core configfs
>> drm_panel_orientation_quirks loop backlight dmi_sysfs crc32_pclmul
>> intel dm_mirror dm_region_hash dm_log dm_mod dax efivarfs
>> [  245.998457] CR2: 0000000000000000
>> [  245.998800] ---[ end trace 0000000000000000 ]---
>> [  246.087916] RIP: 0010:cifs_flush_folio+0x3f/0x100 [cifs]
>> [  246.088794] Code: d2 41 54 49 89 cc 31 c9 55 53 48 89 f3 48 c1 ee 0c
>> 48 83 ec 08 48 8b 7f 30 e8 7d 2c a6 f8 48 3d 00 f0 ff ff 0f 87 a5 00 00
>> 00 <48> 8b 10 48 89 c5 b8 00 10 00 00 f7 c2 00 00 01 00 74 07 0f b6 4d
>> [  246.099436] RSP: 0018:ffffc25441ffbd20 EFLAGS: 00010207
>> [  246.189258] RAX: 0000000000000000 RBX: 0000000000000000 RCX:
>> 0000000000000000
>> [  246.198724] RDX: 0000000000000000 RSI: 0000000000000000 RDI:
>> ffff9ed945eac000
>> [  246.199411] RBP: 00000000000003a1 R08: 0000000000000001 R09:
>> 0000000000000000
>> [  246.298002] R10: 0000000000003e7f R11: 0000000000000000 R12:
>> ffffc25441ffbd90
>> [  246.298687] R13: ffffc25441ffbd88 R14: ffff9ed94af23850 R15:
>> 0000000000000001
>> [  246.299372] FS:  00007f111404d340(0000) GS:ffff9ee7fecc0000(0000)
>> knlGS:0000000000000000
>> [  246.398006] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [  246.398569] CR2: 0000000000000000 CR3: 00000001211fc001 CR4:
>> 00000000007706e0
>> [  246.399254] DR0: 0000000000000000 DR1: 0000000000000000 DR2:
>> 0000000000000000
>> [  246.399934] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7:
>> 0000000000000400
>> [  246.498414] PKRU: 55555554
>> [  246.498698] Kernel panic - not syncing: Fatal exception
>> [  246.500042] Kernel Offset: 0x38000000 from 0xffffffff81000000
>> (relocation range: 0xffffffff80000000-0xffffffffbfffffff)
>> [  246.698094] Rebooting in 5 seconds..
>>
>> Any ideas what is causing this and what the resolution is? This doesn't
>> occur on the upstream/master kernel.
>>
>> Thanks,
>> Suraj
>>
>> On Mon, 2023-12-11 at 13:57 +0100, gregkh@linuxfoundation.org wrote:
>>>
>>> This is a note to let you know that I've just added the patch titled
>>>
>>>     cifs: Fix flushing, invalidation and file size with
>>> copy_file_range()
>>>
>>> to the 6.1-stable tree which can be found at:
>>>    
>>> http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary
>>>
>>> The filename of the patch is:
>>>      cifs-fix-flushing-invalidation-and-file-size-with-
>>> copy_file_range.patch
>>> and it can be found in the queue-6.1 subdirectory.
>>>
>>> If you, or anyone else, feels it should not be added to the stable
>>> tree,
>>> please let <stable@vger.kernel.org> know about it.
>>>
>>>
>>>> From 7b2404a886f8b91250c31855d287e632123e1746 Mon Sep 17 00:00:00
>>>> 2001
>>> From: David Howells <dhowells@redhat.com>
>>> Date: Fri, 1 Dec 2023 00:22:00 +0000
>>> Subject: cifs: Fix flushing, invalidation and file size with
>>> copy_file_range()
>>>
>>> From: David Howells <dhowells@redhat.com>
>>>
>>> commit 7b2404a886f8b91250c31855d287e632123e1746 upstream.
>>>
>>> Fix a number of issues in the cifs filesystem implementation of the
>>> copy_file_range() syscall in cifs_file_copychunk_range().
>>>
>>> Firstly, the invalidation of the destination range is handled
>>> incorrectly:
>>> We shouldn't just invalidate the whole file as dirty data in the file
>>> may
>>> get lost and we can't just call truncate_inode_pages_range() to
>>> invalidate
>>> the destination range as that will erase parts of a partial folio at
>>> each
>>> end whilst invalidating and discarding all the folios in the middle. 
>>> We
>>> need to force all the folios covering the range to be reloaded, but
>>> we
>>> mustn't lose dirty data in them that's not in the destination range.
>>>
>>> Further, we shouldn't simply round out the range to PAGE_SIZE at each
>>> end
>>> as cifs should move to support multipage folios.
>>>
>>> Secondly, there's an issue whereby a write may have extended the file
>>> locally, but not have been written back yet.  This can leaves the
>>> local
>>> idea of the EOF at a later point than the server's EOF.  If a copy
>>> request
>>> is issued, this will fail on the server with STATUS_INVALID_VIEW_SIZE
>>> (which gets translated to -EIO locally) if the copy source extends
>>> past the
>>> server's EOF.
>>>
>>> Fix this by:
>>>
>>>  (0) Flush the source region (already done).  The flush does nothing
>>> and
>>>      the EOF isn't moved if the source region has no dirty data.
>>>
>>>  (1) Move the EOF to the end of the source region if it isn't already
>>> at
>>>      least at this point.  If we can't do this, for instance if the
>>> server
>>>      doesn't support it, just flush the entire source file.
>>>
>>>  (2) Find the folio (if present) at each end of the range, flushing
>>> it and
>>>      increasing the region-to-be-invalidated to cover those in their
>>>      entirety.
>>>
>>>  (3) Fully discard all the folios covering the range as we want them
>>> to be
>>>      reloaded.
>>>
>>>  (4) Then perform the copy.
>>>
>>> Thirdly, set i_size after doing the copychunk_range operation as this
>>> value
>>> may be used by various things internally.  stat() hides the issue
>>> because
>>> setting ->time to 0 causes cifs_getatr() to revalidate the
>>> attributes.
>>>
>>> These were causing the generic/075 xfstest to fail.
>>>
>>> Fixes: 620d8745b35d ("Introduce cifs_copy_file_range()")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: David Howells <dhowells@redhat.com>
>>> cc: Paulo Alcantara <pc@manguebit.com>
>>> cc: Shyam Prasad N <nspmangalore@gmail.com>
>>> cc: Rohith Surabattula <rohiths.msft@gmail.com>
>>> cc: Matthew Wilcox <willy@infradead.org>
>>> cc: Jeff Layton <jlayton@kernel.org>
>>> cc: linux-cifs@vger.kernel.org
>>> cc: linux-mm@kvack.org
>>> Signed-off-by: David Howells <dhowells@redhat.com>
>>> Signed-off-by: Steve French <stfrench@microsoft.com>
>>> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> ---
>>>  fs/smb/client/cifsfs.c |  102
>>> +++++++++++++++++++++++++++++++++++++++++++++++--
>>>  1 file changed, 99 insertions(+), 3 deletions(-)
>>>
>>> --- a/fs/smb/client/cifsfs.c
>>> +++ b/fs/smb/client/cifsfs.c
>>> @@ -1191,6 +1191,72 @@ const struct inode_operations cifs_symli
>>>         .listxattr = cifs_listxattr,
>>>  };
>>>  
>>> +/*
>>> + * Advance the EOF marker to after the source range.
>>> + */
>>> +static int cifs_precopy_set_eof(struct inode *src_inode, struct
>>> cifsInodeInfo *src_cifsi,
>>> +                               struct cifs_tcon *src_tcon,
>>> +                               unsigned int xid, loff_t src_end)
>>> +{
>>> +       struct cifsFileInfo *writeable_srcfile;
>>> +       int rc = -EINVAL;
>>> +
>>> +       writeable_srcfile = find_writable_file(src_cifsi,
>>> FIND_WR_FSUID_ONLY);
>>> +       if (writeable_srcfile) {
>>> +               if (src_tcon->ses->server->ops->set_file_size)
>>> +                       rc = src_tcon->ses->server->ops-
>>>> set_file_size(
>>> +                               xid, src_tcon, writeable_srcfile,
>>> +                               src_inode->i_size, true /* no need to
>>> set sparse */);
>>> +               else
>>> +                       rc = -ENOSYS;
>>> +               cifsFileInfo_put(writeable_srcfile);
>>> +               cifs_dbg(FYI, "SetFSize for copychunk rc = %d\n",
>>> rc);
>>> +       }
>>> +
>>> +       if (rc < 0)
>>> +               goto set_failed;
>>> +
>>> +       netfs_resize_file(&src_cifsi->netfs, src_end);
>>> +       fscache_resize_cookie(cifs_inode_cookie(src_inode), src_end);
>>> +       return 0;
>>> +
>>> +set_failed:
>>> +       return filemap_write_and_wait(src_inode->i_mapping);
>>> +}
>>> +
>>> +/*
>>> + * Flush out either the folio that overlaps the beginning of a range
>>> in which
>>> + * pos resides or the folio that overlaps the end of a range unless
>>> that folio
>>> + * is entirely within the range we're going to invalidate.  We
>>> extend the flush
>>> + * bounds to encompass the folio.
>>> + */
>>> +static int cifs_flush_folio(struct inode *inode, loff_t pos, loff_t
>>> *_fstart, loff_t *_fend,
>>> +                           bool first)
>>> +{
>>> +       struct folio *folio;
>>> +       unsigned long long fpos, fend;
>>> +       pgoff_t index = pos / PAGE_SIZE;
>>> +       size_t size;
>>> +       int rc = 0;
>>> +
>>> +       folio = filemap_get_folio(inode->i_mapping, index);
>>> +       if (IS_ERR(folio))
>>> +               return 0;
>>> +
>>> +       size = folio_size(folio);
>>> +       fpos = folio_pos(folio);
>>> +       fend = fpos + size - 1;
>>> +       *_fstart = min_t(unsigned long long, *_fstart, fpos);
>>> +       *_fend   = max_t(unsigned long long, *_fend, fend);
>>> +       if ((first && pos == fpos) || (!first && pos == fend))
>>> +               goto out;
>>> +
>>> +       rc = filemap_write_and_wait_range(inode->i_mapping, fpos,
>>> fend);
>>> +out:
>>> +       folio_put(folio);
>>> +       return rc;
>>> +}
>>> +
>>>  static loff_t cifs_remap_file_range(struct file *src_file, loff_t
>>> off,
>>>                 struct file *dst_file, loff_t destoff, loff_t len,
>>>                 unsigned int remap_flags)
>>> @@ -1260,10 +1326,12 @@ ssize_t cifs_file_copychunk_range(unsign
>>>  {
>>>         struct inode *src_inode = file_inode(src_file);
>>>         struct inode *target_inode = file_inode(dst_file);
>>> +       struct cifsInodeInfo *src_cifsi = CIFS_I(src_inode);
>>>         struct cifsFileInfo *smb_file_src;
>>>         struct cifsFileInfo *smb_file_target;
>>>         struct cifs_tcon *src_tcon;
>>>         struct cifs_tcon *target_tcon;
>>> +       unsigned long long destend, fstart, fend;
>>>         ssize_t rc;
>>>  
>>>         cifs_dbg(FYI, "copychunk range\n");
>>> @@ -1303,13 +1371,41 @@ ssize_t cifs_file_copychunk_range(unsign
>>>         if (rc)
>>>                 goto unlock;
>>>  
>>> -       /* should we flush first and last page first */
>>> -       truncate_inode_pages(&target_inode->i_data, 0);
>>> +       /* The server-side copy will fail if the source crosses the
>>> EOF marker.
>>> +        * Advance the EOF marker after the flush above to the end of
>>> the range
>>> +        * if it's short of that.
>>> +        */
>>> +       if (src_cifsi->server_eof < off + len) {
>>> +               rc = cifs_precopy_set_eof(src_inode, src_cifsi,
>>> src_tcon, xid, off + len);
>>> +               if (rc < 0)
>>> +                       goto unlock;
>>> +       }
>>> +
>>> +       destend = destoff + len - 1;
>>> +
>>> +       /* Flush the folios at either end of the destination range to
>>> prevent
>>> +        * accidental loss of dirty data outside of the range.
>>> +        */
>>> +       fstart = destoff;
>>> +       fend = destend;
>>> +
>>> +       rc = cifs_flush_folio(target_inode, destoff, &fstart, &fend,
>>> true);
>>> +       if (rc)
>>> +               goto unlock;
>>> +       rc = cifs_flush_folio(target_inode, destend, &fstart, &fend,
>>> false);
>>> +       if (rc)
>>> +               goto unlock;
>>> +
>>> +       /* Discard all the folios that overlap the destination
>>> region. */
>>> +       truncate_inode_pages_range(&target_inode->i_data, fstart,
>>> fend);
>>>  
>>>         rc = file_modified(dst_file);
>>> -       if (!rc)
>>> +       if (!rc) {
>>>                 rc = target_tcon->ses->server->ops-
>>>> copychunk_range(xid,
>>>                         smb_file_src, smb_file_target, off, len,
>>> destoff);
>>> +               if (rc > 0 && destoff + rc >
>>> i_size_read(target_inode))
>>> +                       truncate_setsize(target_inode, destoff + rc);
>>> +       }
>>>  
>>>         file_accessed(src_file);
>>>  
>>>
>>>
>>> Patches currently in stable-queue which might be from
>>> dhowells@redhat.com are
>>>
>>> queue-6.1/cifs-fix-flushing-invalidation-and-file-size-with-
>>> copy_file_range.patch
>>> queue-6.1/cifs-fix-flushing-invalidation-and-file-size-with-
>>> ficlone.patch
>>> queue-6.1/cifs-fix-non-availability-of-dedup-breaking-generic-
>>> 304.patch
>>>
>>
> 
> It looks we had as well some reports in Debian about this regression.
> Copying a file within the same cifs mount seems to trigger the NULL
> pointer dereference:
> 
> [ 1640.742300] BUG: kernel NULL pointer dereference, address: 0000000000000000
> [ 1640.742309] #PF: supervisor read access in kernel mode
> [ 1640.742313] #PF: error_code(0x0000) - not-present page
> [ 1640.742317] PGD 0 P4D 0
> [ 1640.742322] Oops: 0000 [#1] PREEMPT SMP NOPTI
> [ 1640.742327] CPU: 6 PID: 1972 Comm: cp Not tainted 6.1.0-17-amd64 #1  Debian 6.1.69-1
> [ 1640.742333] Hardware name: LENOVO 20XXS1FU00/20XXS1FU00, BIOS N32ET89W (1.65 ) 11/13/2023
> [ 1640.742336] RIP: 0010:cifs_flush_folio+0x3f/0x100 [cifs]
> [ 1640.742412] Code: d2 41 54 49 89 cc 31 c9 55 48 89 f5 48 c1 ee 0c 53 48 83 ec 08 48 8b 7f 30 e8 8d 9a 97 dd 48 3d 00 f0 ff ff 0f 87 a5 00 00 00 <48> 8b 10 48 89 c3 b8 00 10 00 00 f7 c2 00 00 01 00 74 07 0f b6 4b
> [ 1640.742417] RSP: 0018:ffff9eac8181fd18 EFLAGS: 00010207
> [ 1640.742422] RAX: 0000000000000000 RBX: 0000000000000004 RCX: 0000000000000000
> [ 1640.742425] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff8a0a758b0000
> [ 1640.742428] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
> [ 1640.742431] R10: 0000000000000003 R11: ffff8a09c596fa00 R12: ffff9eac8181fd88
> [ 1640.742433] R13: ffff9eac8181fd80 R14: ffff8a0a707bda48 R15: 0000000000000001
> [ 1640.742437] FS:  00007f7dc8764800(0000) GS:ffff8a10ff780000(0000) knlGS:0000000000000000
> [ 1640.742441] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1640.742444] CR2: 0000000000000000 CR3: 000000015eff0003 CR4: 0000000000770ee0
> [ 1640.742448] PKRU: 55555554
> [ 1640.742450] Call Trace:
> [ 1640.742454]  <TASK>
> [ 1640.742459]  ? __die_body.cold+0x1a/0x1f
> [ 1640.742468]  ? page_fault_oops+0xd2/0x2b0
> [ 1640.742476]  ? exc_page_fault+0x70/0x170
> [ 1640.742482]  ? asm_exc_page_fault+0x22/0x30
> [ 1640.742492]  ? cifs_flush_folio+0x3f/0x100 [cifs]
> [ 1640.742558]  ? cifs_flush_folio+0x33/0x100 [cifs]
> [ 1640.742622]  ? cifs_precopy_set_eof+0x2b/0x150 [cifs]
> [ 1640.742688]  cifs_remap_file_range+0x16d/0x680 [cifs]
> [ 1640.742755]  do_clone_file_range+0xe6/0x230
> [ 1640.742762]  vfs_clone_file_range+0x37/0x140
> [ 1640.742767]  ioctl_file_clone+0x49/0xb0
> [ 1640.742774]  do_vfs_ioctl+0x77/0x910
> [ 1640.742780]  __x64_sys_ioctl+0x6e/0xd0
> [ 1640.742785]  do_syscall_64+0x58/0xc0
> [ 1640.742794]  entry_SYSCALL_64_after_hwframe+0x64/0xce
> [ 1640.742801] RIP: 0033:0x7f7dc8900b5b
> [ 1640.742806] Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1c 48 8b 44 24 18 64 48 2b 04 25 28 00 00
> [ 1640.742810] RSP: 002b:00007ffe4b899000 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> [ 1640.742815] RAX: ffffffffffffffda RBX: 0000000000000004 RCX: 00007f7dc8900b5b
> [ 1640.742817] RDX: 0000000000000003 RSI: 0000000040049409 RDI: 0000000000000004
> [ 1640.742820] RBP: 00007ffe4b899440 R08: 00007ffe4b899600 R09: 0000000000000001
> [ 1640.742823] R10: 00007f7dc881a358 R11: 0000000000000246 R12: 0000000000000001
> [ 1640.742825] R13: 00007ffe4b899dc3 R14: 0000000000008000 R15: 0000000000000000
> [ 1640.742831]  </TASK>
> [ 1640.742832] Modules linked in: nls_utf8 cifs cifs_arc4 cifs_md4 dns_resolver fscache netfs ctr ccm rfcomm cmac algif_hash algif_skcipher af_alg snd_seq_dummy snd_hrtimer snd_seq snd_seq_device bnep btusb btrtl btbcm btintel btmtk bluetooth uvcvideo videobuf2_vmalloc videobuf2_memops jitterentropy_rng videobuf2_v4l2 videobuf2_common drbg videodev ansi_cprng mc ecdh_generic ecc qrtr ip6t_REJECT nf_reject_ipv6 xt_hl ip6_tables ip6t_rt snd_ctl_led snd_soc_skl_hda_dsp snd_soc_intel_hda_dsp_common binfmt_misc snd_soc_hdac_hdmi ipt_REJECT snd_sof_probes nf_reject_ipv4 xt_LOG nf_log_syslog xt_comment nft_limit xt_limit xt_addrtype snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic snd_soc_dmic snd_sof_pci_intel_tgl snd_sof_intel_hda_common soundwire_intel soundwire_generic_allocation joydev soundwire_cadence snd_sof_intel_hda x86_pkg_temp_thermal snd_sof_pci intel_powerclamp snd_sof_xtensa_dsp coretemp snd_sof snd_sof_utils snd_soc_hdac_hda kvm_intel snd_hda_ext_core
> [ 1640.742921]  snd_soc_acpi_intel_match xt_tcpudp snd_soc_acpi kvm snd_soc_core xt_conntrack irqbypass nf_conntrack crc32_pclmul snd_compress nf_defrag_ipv6 soundwire_bus nf_defrag_ipv4 ghash_clmulni_intel iwlmvm nft_compat snd_hda_intel sha512_ssse3 snd_intel_dspcfg nf_tables snd_intel_sdw_acpi sha512_generic mac80211 libcrc32c snd_hda_codec hid_multitouch nfnetlink processor_thermal_device_pci_legacy hid_generic sha256_ssse3 iTCO_wdt snd_hda_core processor_thermal_device sha1_ssse3 intel_pmc_bxt iTCO_vendor_support processor_thermal_rfim libarc4 pmt_telemetry rapl thinkpad_acpi snd_hwdep xhci_pci processor_thermal_mbox mei_hdcp watchdog intel_rapl_msr snd_pcm pmt_class nls_ascii nvram intel_cstate processor_thermal_rapl xhci_hcd ucsi_acpi iwlwifi platform_profile snd_timer nls_cp437 intel_uncore i2c_hid_acpi intel_lpss_pci usbcore typec_ucsi ledtrig_audio think_lmi mei_me i2c_i801 intel_rapl_common snd pcspkr vfat i2c_hid intel_lpss roles cfg80211 firmware_attributes_class mei
> [ 1640.743007]  thunderbolt usb_common soundcore wmi_bmof fat int3403_thermal idma64 i2c_smbus intel_vsec typec intel_soc_dts_iosf hid igen6_edac button battery ac rfkill soc_button_array int340x_thermal_zone int3400_thermal intel_hid intel_pmc_core acpi_thermal_rel acpi_tad acpi_pad sparse_keymap msr parport_pc ppdev lp parport loop fuse efi_pstore configfs efivarfs ip_tables x_tables autofs4 ext4 crc16 mbcache jbd2 crc32c_generic dm_crypt dm_mod i915 i2c_algo_bit drm_buddy nvme drm_display_helper nvme_core crc32c_intel t10_pi drm_kms_helper crc64_rocksoft cec crc64 rc_core crc_t10dif ttm crct10dif_generic aesni_intel crct10dif_pclmul psmouse drm evdev crypto_simd cryptd crct10dif_common serio_raw video wmi
> [ 1640.743095] CR2: 0000000000000000
> [ 1640.743098] ---[ end trace 0000000000000000 ]---
> [ 1640.957607] RIP: 0010:cifs_flush_folio+0x3f/0x100 [cifs]
> [ 1640.957681] Code: d2 41 54 49 89 cc 31 c9 55 48 89 f5 48 c1 ee 0c 53 48 83 ec 08 48 8b 7f 30 e8 8d 9a 97 dd 48 3d 00 f0 ff ff 0f 87 a5 00 00 00 <48> 8b 10 48 89 c3 b8 00 10 00 00 f7 c2 00 00 01 00 74 07 0f b6 4b
> [ 1640.957683] RSP: 0018:ffff9eac8181fd18 EFLAGS: 00010207
> [ 1640.957685] RAX: 0000000000000000 RBX: 0000000000000004 RCX: 0000000000000000
> [ 1640.957686] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffff8a0a758b0000
> [ 1640.957687] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000
> [ 1640.957688] R10: 0000000000000003 R11: ffff8a09c596fa00 R12: ffff9eac8181fd88
> [ 1640.957689] R13: ffff9eac8181fd80 R14: ffff8a0a707bda48 R15: 0000000000000001
> [ 1640.957691] FS:  00007f7dc8764800(0000) GS:ffff8a10ff780000(0000) knlGS:0000000000000000
> [ 1640.957692] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 1640.957693] CR2: 0000000000000000 CR3: 000000015eff0003 CR4: 0000000000770ee0
> [ 1640.957695] PKRU: 55555554
> [ 1640.957696] note: cp[1972] exited with irqs disabled
> 
> #regzbot ^introduced 18b02e4343e8f5be6a2f44c7ad9899b385a92730
> #regzbot link: https://bugs.debian.org/1060005
> #regzbot link: https://bugs.debian.org/1060052
> 
> Regards,
> Salvatore
> 
> 

Re: [Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[Salvatore Bonaccorso]

Hi,

On Sat, Jan 06, 2024 at 11:40:58AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> Hi, Thorsten here, the Linux kernel's regression tracker. Top-posting
> for once, to make this easily accessible to everyone.
> 
> Thank's for CCIng the regression list and telling regzbot about the
> issue. There is one important thing that afaics[1] is missing and would
> be really good to know[2]:
> 
> Does this problem also happen in mainline, e.g. with 6.7-rc8?
> 
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
>
> [1] I hope I didn't miss this
> [2] as explained here:
> https://docs.kernel.org/admin-guide/reporting-issues.html
> https://linux-regtracking.leemhuis.info/post/frequent-reasons-why-linux-kernel-bug-reports-are-ignored/#you-reported-a-regression-in-a-stable-or-longterm-series-without-checking-if-mainline-is-affected-as-well

Thanks a lot for replying back. So far I can tell, the regression is
in 6.1.y only and might indicate that some prerequisite changes (maybe
in the folio refactoring?) is missing. The commit identified by Suraj,
7b2404a886f8 ("cifs: Fix flushing, invalidation and file size with
copy_file_range()") from 6.7-rc5 was backported to two of the stable
series, 6.1.68 and 6.6.7.

I did not test mainline specifically (would need to first build) but
did test 6.6.9 based version and the problem is not reproduible there
(and neither with 6.6.10-rc1 which I have build for the RC series
review posting).

For this reason I added to regzbot only "regzbot ^introduced
18b02e4343e8f5be6a2f44c7ad9899b385a92730" which is the commit in
v6.1.68.

Regards,
Salvatore

Re: [Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[Linux regression tracking (Thorsten Leemhuis)]

On 06.01.24 12:34, Salvatore Bonaccorso wrote:
> On Sat, Jan 06, 2024 at 11:40:58AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
>>
>> Does this problem also happen in mainline, e.g. with 6.7-rc8?
> 
> Thanks a lot for replying back. So far I can tell, the regression is
> in 6.1.y only 

Ahh, good to know, thx!

> For this reason I added to regzbot only "regzbot ^introduced
> 18b02e4343e8f5be6a2f44c7ad9899b385a92730" which is the commit in
> v6.1.68.

Which was the totally right thing to do, thx. Guess I sooner or later
will add something like "#regzbot tag notinmainline" to avoid the
ambiguity we just cleared up, but maybe that's overkill.

Ciao, Thorsten!

Re: [Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[Salvatore Bonaccorso]

Hi

Sorry if this is to prematurely to ask already again.

On Sat, Jan 06, 2024 at 01:02:16PM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 06.01.24 12:34, Salvatore Bonaccorso wrote:
> > On Sat, Jan 06, 2024 at 11:40:58AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> >>
> >> Does this problem also happen in mainline, e.g. with 6.7-rc8?
> > 
> > Thanks a lot for replying back. So far I can tell, the regression is
> > in 6.1.y only 
> 
> Ahh, good to know, thx!
> 
> > For this reason I added to regzbot only "regzbot ^introduced
> > 18b02e4343e8f5be6a2f44c7ad9899b385a92730" which is the commit in
> > v6.1.68.
> 
> Which was the totally right thing to do, thx. Guess I sooner or later
> will add something like "#regzbot tag notinmainline" to avoid the
> ambiguity we just cleared up, but maybe that's overkill.

Do we have already a picture on the best move forward? Should the
patch and the what depends on it be reverted or was someone already
able to isolate where the problem comes from specifically for the
6.1.y series? 

Regards,
Salvatore

Re: [Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[gregkh]

On Wed, Jan 10, 2024 at 05:20:27PM +0100, Salvatore Bonaccorso wrote:
> Hi
> 
> Sorry if this is to prematurely to ask already again.
> 
> On Sat, Jan 06, 2024 at 01:02:16PM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> > On 06.01.24 12:34, Salvatore Bonaccorso wrote:
> > > On Sat, Jan 06, 2024 at 11:40:58AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
> > >>
> > >> Does this problem also happen in mainline, e.g. with 6.7-rc8?
> > > 
> > > Thanks a lot for replying back. So far I can tell, the regression is
> > > in 6.1.y only 
> > 
> > Ahh, good to know, thx!
> > 
> > > For this reason I added to regzbot only "regzbot ^introduced
> > > 18b02e4343e8f5be6a2f44c7ad9899b385a92730" which is the commit in
> > > v6.1.68.
> > 
> > Which was the totally right thing to do, thx. Guess I sooner or later
> > will add something like "#regzbot tag notinmainline" to avoid the
> > ambiguity we just cleared up, but maybe that's overkill.
> 
> Do we have already a picture on the best move forward? Should the
> patch and the what depends on it be reverted or was someone already
> able to isolate where the problem comes from specifically for the
> 6.1.y series? 

I guess I can just revert the single commit here?  Can someone send me
the revert that I need to do so as I get it right?

thanks,

greg k-h

Re: [Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[Linux regression tracking (Thorsten Leemhuis)]

On 11.01.24 12:03, gregkh@linuxfoundation.org wrote:
> On Wed, Jan 10, 2024 at 05:20:27PM +0100, Salvatore Bonaccorso wrote:
>> On Sat, Jan 06, 2024 at 01:02:16PM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
>>> On 06.01.24 12:34, Salvatore Bonaccorso wrote:
>>>> On Sat, Jan 06, 2024 at 11:40:58AM +0100, Linux regression tracking (Thorsten Leemhuis) wrote:
>>>>>
>>>>> Does this problem also happen in mainline, e.g. with 6.7-rc8?
>>>> Thanks a lot for replying back. So far I can tell, the regression is
>>>> in 6.1.y only 
>>> Ahh, good to know, thx!
>>>
>>>> For this reason I added to regzbot only "regzbot ^introduced
>>>> 18b02e4343e8f5be6a2f44c7ad9899b385a92730" which is the commit in
>>>> v6.1.68.
>>> Which was the totally right thing to do, thx. Guess I sooner or later
>>> will add something like "#regzbot tag notinmainline" to avoid the
>>> ambiguity we just cleared up, but maybe that's overkill.
>> Do we have already a picture on the best move forward? Should the
>> patch and the what depends on it be reverted or was someone already
>> able to isolate where the problem comes from specifically for the
>> 6.1.y series? 
> I guess I can just revert the single commit here?  Can someone send me
> the revert that I need to do so as I get it right?

Steve what's you opinion on reverting this? From
https://lore.kernel.org/all/CAH2r5mu7e5-ORZbUyutteWVx2Nk6FPHfx7mMGCWSCEBAO6tdqg@mail.gmail.com/
it looks like you added the stable tag to the culprit on purpose.

FWIW, this thread stared there (just in case you missed earlier msgs):
https://lore.kernel.org/all/a76b370f93cb928c049b94e1fde0d2da506dfcb2.camel@amazon.com/

Ciao, Thorsten

Re: [Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[David Howells]

gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> wrote:

> I guess I can just revert the single commit here?  Can someone send me
> the revert that I need to do so as I get it right?

In cifs_flush_folio() the error check for filemap_get_folio() just needs
changing to check !folio instead of IS_ERR(folio).

David

Re: [Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]

Here is a patch similar to what David suggested.  Seems
straightforward fix.  See attached.
I did limited testing on it tonight with 6.1 (will do more tomorrow,
but feedback welcome) but it did fix the regression in xfstest
generic/001 mentioned in this thread.




On Fri, Jan 12, 2024 at 8:26 AM David Howells <dhowells@redhat.com> wrote:
>
> gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> wrote:
>
> > I guess I can just revert the single commit here?  Can someone send me
> > the revert that I need to do so as I get it right?
>
> In cifs_flush_folio() the error check for filemap_get_folio() just needs
> changing to check !folio instead of IS_ERR(folio).
>
> David
>
>


--
Thanks,

Steve

[-- Attachment #2: 0001-cifs-fix-flushing-folio-regression-for-6.1-backport.patch --]
[-- Type: text/x-patch, Size: 1157 bytes --]

From ba288a873fb8ac3d1bf5563366558a905620c071 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Fri, 12 Jan 2024 23:08:51 -0600
Subject: [PATCH] cifs: fix flushing folio regression for 6.1 backport

filemap_get_folio works differenty in 6.1 vs. later kernels
(returning NULL in 6.1 instead of an error).  Add
this minor correction which addresses the regression in the patch:
  cifs: Fix flushing, invalidation and file size with copy_file_range()

Suggested-by: David Howells <dhowells@redhat.com>
Reported-by: Salvatore Bonaccorso <carnil@debian.org>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/smb/client/cifsfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
index 2e15b182e59f..ac0b7f229a23 100644
--- a/fs/smb/client/cifsfs.c
+++ b/fs/smb/client/cifsfs.c
@@ -1240,7 +1240,7 @@ static int cifs_flush_folio(struct inode *inode, loff_t pos, loff_t *_fstart, lo
 	int rc = 0;
 
 	folio = filemap_get_folio(inode->i_mapping, index);
-	if (IS_ERR(folio))
+	if ((!folio) || (IS_ERR(folio)))
 		return 0;
 
 	size = folio_size(folio);
-- 
2.40.1

Re: [Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[gregkh]

On Fri, Jan 12, 2024 at 11:20:53PM -0600, Steve French wrote:
> Here is a patch similar to what David suggested.  Seems
> straightforward fix.  See attached.
> I did limited testing on it tonight with 6.1 (will do more tomorrow,
> but feedback welcome) but it did fix the regression in xfstest
> generic/001 mentioned in this thread.

Thanks, now queued up!

greg k-h

Re: [Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[Salvatore Bonaccorso]

Hi,

On Fri, Jan 12, 2024 at 11:20:53PM -0600, Steve French wrote:
> Here is a patch similar to what David suggested.  Seems
> straightforward fix.  See attached.
> I did limited testing on it tonight with 6.1 (will do more tomorrow,
> but feedback welcome) but it did fix the regression in xfstest
> generic/001 mentioned in this thread.
> 
> 
> 
> 
> On Fri, Jan 12, 2024 at 8:26 AM David Howells <dhowells@redhat.com> wrote:
> >
> > gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> wrote:
> >
> > > I guess I can just revert the single commit here?  Can someone send me
> > > the revert that I need to do so as I get it right?
> >
> > In cifs_flush_folio() the error check for filemap_get_folio() just needs
> > changing to check !folio instead of IS_ERR(folio).
> >
> > David
> >
> >
> 
> 
> --
> Thanks,
> 
> Steve

> From ba288a873fb8ac3d1bf5563366558a905620c071 Mon Sep 17 00:00:00 2001
> From: Steve French <stfrench@microsoft.com>
> Date: Fri, 12 Jan 2024 23:08:51 -0600
> Subject: [PATCH] cifs: fix flushing folio regression for 6.1 backport
> 
> filemap_get_folio works differenty in 6.1 vs. later kernels
> (returning NULL in 6.1 instead of an error).  Add
> this minor correction which addresses the regression in the patch:
>   cifs: Fix flushing, invalidation and file size with copy_file_range()
> 
> Suggested-by: David Howells <dhowells@redhat.com>
> Reported-by: Salvatore Bonaccorso <carnil@debian.org>
> Signed-off-by: Steve French <stfrench@microsoft.com>
> ---
>  fs/smb/client/cifsfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> index 2e15b182e59f..ac0b7f229a23 100644
> --- a/fs/smb/client/cifsfs.c
> +++ b/fs/smb/client/cifsfs.c
> @@ -1240,7 +1240,7 @@ static int cifs_flush_folio(struct inode *inode, loff_t pos, loff_t *_fstart, lo
>  	int rc = 0;
>  
>  	folio = filemap_get_folio(inode->i_mapping, index);
> -	if (IS_ERR(folio))
> +	if ((!folio) || (IS_ERR(folio)))
>  		return 0;
>  
>  	size = folio_size(folio);

I was able to test the patch with the case from the Debian bugreport
and seems to resolve the issue. Even if late, as Greg just queued up
already:

Tested-by: Salvatore Bonaccorso <carnil@debian.org>

I wonder, but cannot(!) confirm, might there have been other such
backports in 6.1.y which are incorrect? At least it does not seem the
case so far for other uses of checking IS_ERR(folio) in v6.1.72 where
were using filemap_get_folio. So I guess the other cases are all okay.
In v6.1.72 finding those:

block/partitions/core.c-
block/partitions/core.c-        folio = read_mapping_folio(mapping, n >> PAGE_SECTORS_SHIFT, NULL);
block/partitions/core.c:        if (IS_ERR(folio))
--
drivers/scsi/scsicam.c-
drivers/scsi/scsicam.c- folio = read_mapping_folio(mapping, 0, NULL);
drivers/scsi/scsicam.c: if (IS_ERR(folio))
--
fs/ceph/addr.c-
fs/ceph/addr.c- folio = read_mapping_folio(inode->i_mapping, 0, file);
fs/ceph/addr.c: if (IS_ERR(folio)) {
--
fs/erofs/data.c-                folio = read_cache_folio(mapping, index, NULL, NULL);
fs/erofs/data.c-                memalloc_nofs_restore(nofs_flag);
fs/erofs/data.c:                if (IS_ERR(folio))
--
fs/ext2/dir.c-  struct folio *folio = read_mapping_folio(mapping, n, NULL);
fs/ext2/dir.c-
fs/ext2/dir.c:  if (IS_ERR(folio))
--
fs/smb/client/cifsfs.c-
fs/smb/client/cifsfs.c- folio = filemap_get_folio(inode->i_mapping, index);
fs/smb/client/cifsfs.c: if (IS_ERR(folio))
--
fs/verity/enable.c-                     page_cache_sync_ra(&ractl, remaining_pages);
fs/verity/enable.c-             folio = read_cache_folio(ractl.mapping, index, NULL, file);
fs/verity/enable.c:             if (IS_ERR(folio))
--
mm/filemap.c-
mm/filemap.c-   folio = do_read_cache_folio(mapping, index, filler, file, gfp);
mm/filemap.c:   if (IS_ERR(folio))
--
mm/shmem.c-     huge_gfp = limit_gfp_mask(huge_gfp, gfp);
mm/shmem.c-     folio = shmem_alloc_and_acct_folio(huge_gfp, inode, index, true);
mm/shmem.c:     if (IS_ERR(folio)) {
--
mm/shmem.c-             folio = shmem_alloc_and_acct_folio(gfp, inode, index, false);
mm/shmem.c-     }
mm/shmem.c:     if (IS_ERR(folio)) {

where only fs/smb/client/cifsfs.c was using filemap_get_folio() in this context.

Regards,
Salvatore

Re: [Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[gregkh]

On Sat, Jan 13, 2024 at 10:31:46AM +0100, Salvatore Bonaccorso wrote:
> Hi,
> 
> On Fri, Jan 12, 2024 at 11:20:53PM -0600, Steve French wrote:
> > Here is a patch similar to what David suggested.  Seems
> > straightforward fix.  See attached.
> > I did limited testing on it tonight with 6.1 (will do more tomorrow,
> > but feedback welcome) but it did fix the regression in xfstest
> > generic/001 mentioned in this thread.
> > 
> > 
> > 
> > 
> > On Fri, Jan 12, 2024 at 8:26 AM David Howells <dhowells@redhat.com> wrote:
> > >
> > > gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> wrote:
> > >
> > > > I guess I can just revert the single commit here?  Can someone send me
> > > > the revert that I need to do so as I get it right?
> > >
> > > In cifs_flush_folio() the error check for filemap_get_folio() just needs
> > > changing to check !folio instead of IS_ERR(folio).
> > >
> > > David
> > >
> > >
> > 
> > 
> > --
> > Thanks,
> > 
> > Steve
> 
> > From ba288a873fb8ac3d1bf5563366558a905620c071 Mon Sep 17 00:00:00 2001
> > From: Steve French <stfrench@microsoft.com>
> > Date: Fri, 12 Jan 2024 23:08:51 -0600
> > Subject: [PATCH] cifs: fix flushing folio regression for 6.1 backport
> > 
> > filemap_get_folio works differenty in 6.1 vs. later kernels
> > (returning NULL in 6.1 instead of an error).  Add
> > this minor correction which addresses the regression in the patch:
> >   cifs: Fix flushing, invalidation and file size with copy_file_range()
> > 
> > Suggested-by: David Howells <dhowells@redhat.com>
> > Reported-by: Salvatore Bonaccorso <carnil@debian.org>
> > Signed-off-by: Steve French <stfrench@microsoft.com>
> > ---
> >  fs/smb/client/cifsfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> > index 2e15b182e59f..ac0b7f229a23 100644
> > --- a/fs/smb/client/cifsfs.c
> > +++ b/fs/smb/client/cifsfs.c
> > @@ -1240,7 +1240,7 @@ static int cifs_flush_folio(struct inode *inode, loff_t pos, loff_t *_fstart, lo
> >  	int rc = 0;
> >  
> >  	folio = filemap_get_folio(inode->i_mapping, index);
> > -	if (IS_ERR(folio))
> > +	if ((!folio) || (IS_ERR(folio)))
> >  		return 0;
> >  
> >  	size = folio_size(folio);
> 
> I was able to test the patch with the case from the Debian bugreport
> and seems to resolve the issue. Even if late, as Greg just queued up
> already:
> 
> Tested-by: Salvatore Bonaccorso <carnil@debian.org>

Thanks, I've added your tested-by to the patch now.

greg k-h

Re: [Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[Matthew Wilcox]

On Fri, Jan 12, 2024 at 11:20:53PM -0600, Steve French wrote:
> Here is a patch similar to what David suggested.  Seems

Similar to, but worse.

> +++ b/fs/smb/client/cifsfs.c
> @@ -1240,7 +1240,7 @@ static int cifs_flush_folio(struct inode *inode, loff_t pos, loff_t *_fstart, lo
>  	int rc = 0;
>  
>  	folio = filemap_get_folio(inode->i_mapping, index);
> -	if (IS_ERR(folio))
> +	if ((!folio) || (IS_ERR(folio)))
>  		return 0;

filemap_get_folio() cannot return an err_ptr in 6.1, so this should
simply be:

-	if (IS_ERR(folio))
+	if (!folio)

Re: [Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[Matthew Wilcox]

On Sat, Jan 13, 2024 at 11:08:00AM -0600, Steve French wrote:
> I thought that it was "safer" since if it was misapplied to version where
> new folio rc behavior it wouldn't regress anything

There are only three versions where this patch can be applied: 6.7, 6.6
and 6.1.  AIUI it's a backport from 6.7, it's already applied to 6.6,
and it misapplies to 6.1.  So this kind of belt-and-braces approach is
unnecessary.

Re: [Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[Greg KH]

On Sat, Jan 13, 2024 at 05:51:19PM +0000, Matthew Wilcox wrote:
> On Sat, Jan 13, 2024 at 11:08:00AM -0600, Steve French wrote:
> > I thought that it was "safer" since if it was misapplied to version where
> > new folio rc behavior it wouldn't regress anything
> 
> There are only three versions where this patch can be applied: 6.7, 6.6
> and 6.1.  AIUI it's a backport from 6.7, it's already applied to 6.6,
> and it misapplies to 6.1.  So this kind of belt-and-braces approach is
> unnecessary.
> 

Agreed, I've fixed this up now, thanks all!

greg k-h

Re: [Regression 6.1.y] From "cifs: Fix flushing, invalidation and file size with copy_file_range()"

[Steve French]

tested out fine so far

On Sat, Jan 13, 2024 at 3:41 AM gregkh@linuxfoundation.org
<gregkh@linuxfoundation.org> wrote:
>
> On Sat, Jan 13, 2024 at 10:31:46AM +0100, Salvatore Bonaccorso wrote:
> > Hi,
> >
> > On Fri, Jan 12, 2024 at 11:20:53PM -0600, Steve French wrote:
> > > Here is a patch similar to what David suggested.  Seems
> > > straightforward fix.  See attached.
> > > I did limited testing on it tonight with 6.1 (will do more tomorrow,
> > > but feedback welcome) but it did fix the regression in xfstest
> > > generic/001 mentioned in this thread.
> > >
> > >
> > >
> > >
> > > On Fri, Jan 12, 2024 at 8:26 AM David Howells <dhowells@redhat.com> wrote:
> > > >
> > > > gregkh@linuxfoundation.org <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > > I guess I can just revert the single commit here?  Can someone send me
> > > > > the revert that I need to do so as I get it right?
> > > >
> > > > In cifs_flush_folio() the error check for filemap_get_folio() just needs
> > > > changing to check !folio instead of IS_ERR(folio).
> > > >
> > > > David
> > > >
> > > >
> > >
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
> >
> > > From ba288a873fb8ac3d1bf5563366558a905620c071 Mon Sep 17 00:00:00 2001
> > > From: Steve French <stfrench@microsoft.com>
> > > Date: Fri, 12 Jan 2024 23:08:51 -0600
> > > Subject: [PATCH] cifs: fix flushing folio regression for 6.1 backport
> > >
> > > filemap_get_folio works differenty in 6.1 vs. later kernels
> > > (returning NULL in 6.1 instead of an error).  Add
> > > this minor correction which addresses the regression in the patch:
> > >   cifs: Fix flushing, invalidation and file size with copy_file_range()
> > >
> > > Suggested-by: David Howells <dhowells@redhat.com>
> > > Reported-by: Salvatore Bonaccorso <carnil@debian.org>
> > > Signed-off-by: Steve French <stfrench@microsoft.com>
> > > ---
> > >  fs/smb/client/cifsfs.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> > > index 2e15b182e59f..ac0b7f229a23 100644
> > > --- a/fs/smb/client/cifsfs.c
> > > +++ b/fs/smb/client/cifsfs.c
> > > @@ -1240,7 +1240,7 @@ static int cifs_flush_folio(struct inode *inode, loff_t pos, loff_t *_fstart, lo
> > >     int rc = 0;
> > >
> > >     folio = filemap_get_folio(inode->i_mapping, index);
> > > -   if (IS_ERR(folio))
> > > +   if ((!folio) || (IS_ERR(folio)))
> > >             return 0;
> > >
> > >     size = folio_size(folio);
> >
> > I was able to test the patch with the case from the Debian bugreport
> > and seems to resolve the issue. Even if late, as Greg just queued up
> > already:
> >
> > Tested-by: Salvatore Bonaccorso <carnil@debian.org>
>
> Thanks, I've added your tested-by to the patch now.
>
> greg k-h



-- 
Thanks,

Steve