Looks good to me. Acked-by: Ronnie Sahlberg <lsahlber@xxxxxxxxxx> On Wed, Jun 16, 2021 at 2:44 AM Thiago Rafael Becker <trbecker@xxxxxxxxx> wrote: > > According to the investigation performed by Jacob Shivers at Red Hat, > cifs_lookup and cifs_readdir leak EAGAIN when the user session is > deleted on the server. Fix this issue by implementing a retry with > limits, as is implemented in cifs_revalidate_dentry_attr. > > Reproducer based on the work by Jacob Shivers: > > ~~~ > $ cat readdir-cifs-test.sh > #!/bin/bash > > # Install and configure powershell and sshd on the windows > # server as descibed in > # https://docs.microsoft.com/en-us/windows-server/administration/openssh/openssh_overview > # This script uses expect(1) > > USER=dude > SERVER=192.168.0.2 > RPATH=root > PASS='password' > > function debug_funcs { > for line in $@ ; do > echo "func $line +p" > /sys/kernel/debug/dynamic_debug/control > done > } > > function setup { > echo 1 > /proc/fs/cifs/cifsFYI > debug_funcs wait_for_compound_request \ > smb2_query_dir_first cifs_readdir \ > compound_send_recv cifs_reconnect_tcon \ > generic_ip_connect cifs_reconnect \ > smb2_reconnect_server smb2_reconnect \ > cifs_readv_from_socket cifs_readv_receive > tcpdump -i eth0 -w cifs.pcap host 192.168.2.182 & sleep 5 > dmesg -C > } > > function test_call { > if [[ $1 == 1 ]] ; then > tracer="strace -tt -f -s 4096 -o trace-$(date -Iseconds).txt" > fi > # Change the command here to anything apropriate > $tracer ls $2 > /dev/null > res=$? > if [[ $1 == 1 ]] ; then > if [[ $res == 0 ]] ; then > 1>&2 echo success > else > 1>&2 echo "failure ($res)" > fi > fi > } > > mountpoint /mnt > /dev/null || mount -t cifs -o username=$USER,pass=$PASS //$SERVER/$RPATH /mnt > > test_call 0 /mnt/ > > /usr/bin/expect << EOF > set timeout 60 > > spawn ssh $USER@$SERVER > > expect "yes/no" { > send "yes\r" > expect "*?assword" { send "$PASS\r" } > } "*?assword" { send "$PASS\r" } > > expect ">" { send "powershell close-smbsession -force\r" } > expect ">" { send "exit\r" } > expect eof > EOF > > sysctl -w vm.drop_caches=2 > /dev/null > sysctl -w vm.drop_caches=2 > /dev/null > > setup > > test_call 1 /mnt/ > ~~~ > > Signed-off-by: Thiago Rafael Becker <trbecker@xxxxxxxxx> > --- > fs/cifs/dir.c | 4 ++++ > fs/cifs/smb2ops.c | 5 +++++ > 2 files changed, 9 insertions(+) > > diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c > index 6bcd3e8f7cda..7c641f9a3dac 100644 > --- a/fs/cifs/dir.c > +++ b/fs/cifs/dir.c > @@ -630,6 +630,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, > struct inode *newInode = NULL; > const char *full_path; > void *page; > + int retry_count = 0; > > xid = get_xid(); > > @@ -673,6 +674,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, > cifs_dbg(FYI, "Full path: %s inode = 0x%p\n", > full_path, d_inode(direntry)); > > +again: > if (pTcon->posix_extensions) > rc = smb311_posix_get_inode_info(&newInode, full_path, parent_dir_inode->i_sb, xid); > else if (pTcon->unix_ext) { > @@ -687,6 +689,8 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry, > /* since paths are not looked up by component - the parent > directories are presumed to be good here */ > renew_parental_timestamps(direntry); > + } else if (rc == -EAGAIN && retry_count++ < 10) { > + goto again; > } else if (rc == -ENOENT) { > cifs_set_time(direntry, jiffies); > newInode = NULL; > diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c > index 21ef51d338e0..d241e6af8fe4 100644 > --- a/fs/cifs/smb2ops.c > +++ b/fs/cifs/smb2ops.c > @@ -2325,6 +2325,7 @@ smb2_query_dir_first(const unsigned int xid, struct cifs_tcon *tcon, > struct smb2_query_directory_rsp *qd_rsp = NULL; > struct smb2_create_rsp *op_rsp = NULL; > struct TCP_Server_Info *server = cifs_pick_channel(tcon->ses); > + int retry_count = 0; > > utf16_path = cifs_convert_path_to_utf16(path, cifs_sb); > if (!utf16_path) > @@ -2372,10 +2373,14 @@ smb2_query_dir_first(const unsigned int xid, struct cifs_tcon *tcon, > > smb2_set_related(&rqst[1]); > > +again: > rc = compound_send_recv(xid, tcon->ses, server, > flags, 2, rqst, > resp_buftype, rsp_iov); > > + if (rc == -EAGAIN && retry_count++ < 10) > + goto again; > + > /* If the open failed there is nothing to do */ > op_rsp = (struct smb2_create_rsp *)rsp_iov[0].iov_base; > if (op_rsp == NULL || op_rsp->sync_hdr.Status != STATUS_SUCCESS) { > -- > 2.31.1 >
