[2.6.30-rc6] cifs_close: NULL pointer dereference

[2.6.30-rc6] cifs_close: NULL pointer dereference

[Luca Tettamanti]

Hello,
I just hit a NULL pointer dereference in cifs_close while accessing a file on a
remote Samba shared directory. The bug is reproducible, a simple:
touch foo; cat foo
is suffient to cause the OOPS. The machine is running kernel from git (1d80cac
- almost rc6), SMP w/ PREEMP. The remote server is running Samba 3.3.3.
I don't use CIFS frequently, but at some point it was certainly working; I can
try a bisection if you want.

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: [<ffffffffa05f105c>] cifs_close+0x1c1/0x2e8 [cifs]
PGD 9205f067 PUD 8f56c067 PMD 0 
Oops: 0002 [#1] PREEMPT SMP 
last sysfs file: /sys/devices/platform/coretemp.0/temp1_input
CPU 0 
Modules linked in: nls_iso8859_1 cifs radeon drm i2c_core af_packet binfmt_misc rfcomm l2cap kvm_intel kvm ipv6 acpi_cpufreq cpufreq_userspace cpufreq_stats cpufreq_conservative cpufreq_powersave cpufreq_ondemand freq_table deflate zlib_deflate twofish_x86_64 twofish_common aes_x86_64 aes_generic blowfish des_generic cbc sha256_generic sha1_generic af_key ext3 jbd coretemp hwmon microcode loop arc4 snd_hda_codec_realtek ecb btusb snd_hda_intel snd_hda_codec iwlagn bluetooth snd_hwdep iwlcore snd_pcm snd_seq rfkill snd_timer snd_seq_device mac80211 snd video soundcore snd_page_alloc psmouse cfg80211 pcspkr evdev asus_laptop output rtc_cmos rtc_core rtc_lib processor battery ac button ext4 mbcache jbd2 crc16 usbhid hid dm_mod sg sd_mod sr_mod cdrom ahci ata_piix ohci1394 sdhci_pci sdhci ieee1394 mmc_core led_class uhci_hcd libata scsi_mod ehci_hcd intel_agp usbcore thermal fan unix
Pid: 17851, comm: cat Not tainted 2.6.30-rc5-00112-g1d80cac #110 F3Sa                
RIP: 0010:[<ffffffffa05f105c>]  [<ffffffffa05f105c>] cifs_close+0x1c1/0x2e8 [cifs]
RSP: 0018:ffff880098537e88  EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff8800bdbe7100 RCX: ffff880098537e08
RDX: 0000000000000000 RSI: 00000000fffffff7 RDI: ffffffffa0616bc4
RBP: ffff880098537ec8 R08: ffff88009573c870 R09: ffff88009227c940
R10: ffff880098537c58 R11: ffff88008f65dad0 R12: 0000000000000000
R13: 000000000000000a R14: 0000000000000000 R15: ffff8800bdbe7170
FS:  00007f7e12bba6f0(0000) GS:ffff88000101c000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 0000000000000008 CR3: 000000008f589000 CR4: 00000000000026e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process cat (pid: 17851, threadinfo ffff880098536000, task ffff88008f4c2b80)
Stack:
 ffff8800922b5900 ffff88006e019008 00000c216e0191f0 0000000000000010
 ffff88006e019008 ffff8800922b5900 ffff88006e019008 ffff8800be725100
 ffff880098537f08 ffffffff802ad55f ffff88006e0cdc00 ffff8800922b5900
Call Trace:
 [<ffffffff802ad55f>] __fput+0xeb/0x1ac
 [<ffffffff802ad638>] fput+0x18/0x1a
 [<ffffffff802aa94c>] filp_close+0x67/0x72
 [<ffffffff802aa9fc>] sys_close+0xa5/0xe4
 [<ffffffff8020bbab>] system_call_fastpath+0x16/0x1b
Code: 89 ef 45 31 e4 e8 f3 63 e7 df 41 bd 0a 00 00 00 48 c7 c7 c4 6b 61 a0 e8 b3 7f e7 df 48 8b 53 10 48 8b 43 18 48 c7 c7 c4 6b 61 a0 <48> 89 42 08 48 89 10 48 c7 43 18 00 02 20 00 48 8b 13 48 8b 43 
RIP  [<ffffffffa05f105c>] cifs_close+0x1c1/0x2e8 [cifs]
 RSP <ffff880098537e88>
CR2: 0000000000000008
---[ end trace c92994e7fd5bf7a2 ]---
note: cat[17851] exited with preempt_count 1

Re: [2.6.30-rc6] cifs_close: NULL pointer dereference

[Luca Tettamanti]

On Sat, May 16, 2009 at 06:28:13PM +0200, Luca Tettamanti wrote:
> Hello,
> I just hit a NULL pointer dereference in cifs_close while accessing a file on a
> remote Samba shared directory.
[...]
> BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> IP: [<ffffffffa05f105c>] cifs_close+0x1c1/0x2e8 [cifs]

Ok, I've reproduced this on a machine with a serial port :)

0xffffffff803574e0 in list_del (entry=0xffff8800ac184210)
at /home/kronos/src/linux-2.6.git/lib/list_debug.c:46
46              WARN(entry->prev->next != entry,
(gdb) bt
#0  0xffffffff803574e0 in list_del (entry=0xffff8800ac184210)
    at /home/kronos/src/linux-2.6.git/lib/list_debug.c:46
#1  0xffffffff80319588 in cifs_close (inode=0xffff8800b8d5f088, file=0xffff8800ad8f6d00)
    at /home/kronos/src/linux-2.6.git/fs/cifs/file.c:670
#2  0xffffffff8029d95e in __fput (file=0xffff8800ad8f6d00)
    at /home/kronos/src/linux-2.6.git/fs/file_table.c:281
#3  0xffffffff8029da32 in fput (file=0xffff8800ac184210)
    at /home/kronos/src/linux-2.6.git/fs/file_table.c:227
#4  0xffffffff8029ad30 in filp_close (filp=0xffff8800ad8f6d00, id=0xffff880037a3e080)
    at /home/kronos/src/linux-2.6.git/fs/open.c:1108
#5  0xffffffff8029ade0 in sys_close (fd=0) 
    at /home/kronos/src/linux-2.6.git/fs/open.c:1137
#6  0xffffffff802271e4 in sysenter_dispatch ()
    at /home/kronos/src/linux-2.6.git/arch/x86/ia32/ia32entry.S:161
#7  0x0000000000000000 in ?? ()

(gdb) f 1
#1  0xffffffff80319588 in cifs_close (inode=0xffff8800b8d5f088, file=0xffff8800ad8f6d00)
    at /home/kronos/src/linux-2.6.git/fs/cifs/file.c:670
670                     list_del(&pSMBFile->flist);

(gdb) p *&pSMBFile->flist
$2 = {next = 0x0, prev = 0x0}

(gdb) p *&pSMBFile->tlist
$5 = {next = 0x0, prev = 0x0}

So both flist and tlist were not initilized in cifs_open.

The content of the whole pSMBFile structure:

(gdb) p *pSMBFile
$6 = {tlist = {next = 0x0, prev = 0x0}, flist = {next = 0x0, prev = 0x0}, uid = 0, pid = 5322, netfid = 13880,
  pfile = 0xffff8800ad8f6d00, pInode = 0xffff8800b8d5f088, lock_mutex = {count = {counter = 1}, wait_lock = {raw_lock = {
        slock = 514}, magic = 3735899821, owner_cpu = 4294967295, owner = 0xffffffffffffffff, dep_map = {
        key = 0xffffffff806a5c98, class_cache = 0x0, name = 0xffffffff80537308 "&lock->wait_lock"}}, wait_list = {
      next = 0xffff8800ac184278, prev = 0xffff8800ac184278}, owner = 0x0, name = 0x0, magic = 0xffff8800ac184240, dep_map = {
      key = 0xffffffff80d7ff50, class_cache = 0xffffffff80835400, name = 0xffffffff8054cdfa "&private_data->lock_mutex"}},
  llist = {next = 0xffff8800ac1842b8, prev = 0xffff8800ac1842b8}, closePend = true, invalidHandle = false,
  messageMode = false, wrtPending = {counter = 0}, fh_mutex = {count = {counter = 1}, wait_lock = {raw_lock = {slock = 0},
      magic = 3735899821, owner_cpu = 4294967295, owner = 0xffffffffffffffff, dep_map = {key = 0xffffffff806a5c98,
        class_cache = 0x0, name = 0xffffffff80537308 "&lock->wait_lock"}}, wait_list = {next = 0xffff8800ac184308,
      prev = 0xffff8800ac184308}, owner = 0x0, name = 0x0, magic = 0xffff8800ac1842d0, dep_map = {key = 0xffffffff80d7ff58,
      class_cache = 0x0, name = 0xffffffff8054cde2 "&private_data->fh_mutex"}}, srch_inf = {index_of_last_entry = 0,
    entries_in_buffer = 0, info_level = 0, resume_key = 0, ntwrk_buf_start = 0x0, srch_entries_start = 0x0, last_entry = 0x0,
    presume_name = 0x0, resume_name_len = 0, endOfSearch = false, emptyDir = false, unicode = false, smallBuf = false}}


Luca

Re: [linux-cifs-client] Re: [2.6.30-rc6] cifs_close: NULL pointer dereference

[Jeff Layton]

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

On Sat, 16 May 2009 22:55:58 +0200
Luca Tettamanti <kronos.it@gmail.com> wrote:

> On Sat, May 16, 2009 at 06:28:13PM +0200, Luca Tettamanti wrote:
> > Hello,
> > I just hit a NULL pointer dereference in cifs_close while accessing a file on a
> > remote Samba shared directory.
> [...]
> > BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> > IP: [<ffffffffa05f105c>] cifs_close+0x1c1/0x2e8 [cifs]
> 
> Ok, I've reproduced this on a machine with a serial port :)
> 
> 0xffffffff803574e0 in list_del (entry=0xffff8800ac184210)
> at /home/kronos/src/linux-2.6.git/lib/list_debug.c:46
> 46              WARN(entry->prev->next != entry,
> (gdb) bt
> #0  0xffffffff803574e0 in list_del (entry=0xffff8800ac184210)
>     at /home/kronos/src/linux-2.6.git/lib/list_debug.c:46
> #1  0xffffffff80319588 in cifs_close (inode=0xffff8800b8d5f088, file=0xffff8800ad8f6d00)
>     at /home/kronos/src/linux-2.6.git/fs/cifs/file.c:670
> #2  0xffffffff8029d95e in __fput (file=0xffff8800ad8f6d00)
>     at /home/kronos/src/linux-2.6.git/fs/file_table.c:281
> #3  0xffffffff8029da32 in fput (file=0xffff8800ac184210)
>     at /home/kronos/src/linux-2.6.git/fs/file_table.c:227
> #4  0xffffffff8029ad30 in filp_close (filp=0xffff8800ad8f6d00, id=0xffff880037a3e080)
>     at /home/kronos/src/linux-2.6.git/fs/open.c:1108
> #5  0xffffffff8029ade0 in sys_close (fd=0) 
>     at /home/kronos/src/linux-2.6.git/fs/open.c:1137
> #6  0xffffffff802271e4 in sysenter_dispatch ()
>     at /home/kronos/src/linux-2.6.git/arch/x86/ia32/ia32entry.S:161
> #7  0x0000000000000000 in ?? ()
> 
> (gdb) f 1
> #1  0xffffffff80319588 in cifs_close (inode=0xffff8800b8d5f088, file=0xffff8800ad8f6d00)
>     at /home/kronos/src/linux-2.6.git/fs/cifs/file.c:670
> 670                     list_del(&pSMBFile->flist);
> 
> (gdb) p *&pSMBFile->flist
> $2 = {next = 0x0, prev = 0x0}
> 
> (gdb) p *&pSMBFile->tlist
> $5 = {next = 0x0, prev = 0x0}
> 
> So both flist and tlist were not initilized in cifs_open.
> 
> The content of the whole pSMBFile structure:
> 
> (gdb) p *pSMBFile
> $6 = {tlist = {next = 0x0, prev = 0x0}, flist = {next = 0x0, prev = 0x0}, uid = 0, pid = 5322, netfid = 13880,
>   pfile = 0xffff8800ad8f6d00, pInode = 0xffff8800b8d5f088, lock_mutex = {count = {counter = 1}, wait_lock = {raw_lock = {
>         slock = 514}, magic = 3735899821, owner_cpu = 4294967295, owner = 0xffffffffffffffff, dep_map = {
>         key = 0xffffffff806a5c98, class_cache = 0x0, name = 0xffffffff80537308 "&lock->wait_lock"}}, wait_list = {
>       next = 0xffff8800ac184278, prev = 0xffff8800ac184278}, owner = 0x0, name = 0x0, magic = 0xffff8800ac184240, dep_map = {
>       key = 0xffffffff80d7ff50, class_cache = 0xffffffff80835400, name = 0xffffffff8054cdfa "&private_data->lock_mutex"}},
>   llist = {next = 0xffff8800ac1842b8, prev = 0xffff8800ac1842b8}, closePend = true, invalidHandle = false,
>   messageMode = false, wrtPending = {counter = 0}, fh_mutex = {count = {counter = 1}, wait_lock = {raw_lock = {slock = 0},
>       magic = 3735899821, owner_cpu = 4294967295, owner = 0xffffffffffffffff, dep_map = {key = 0xffffffff806a5c98,
>         class_cache = 0x0, name = 0xffffffff80537308 "&lock->wait_lock"}}, wait_list = {next = 0xffff8800ac184308,
>       prev = 0xffff8800ac184308}, owner = 0x0, name = 0x0, magic = 0xffff8800ac1842d0, dep_map = {key = 0xffffffff80d7ff58,
>       class_cache = 0x0, name = 0xffffffff8054cde2 "&private_data->fh_mutex"}}, srch_inf = {index_of_last_entry = 0,
>     entries_in_buffer = 0, info_level = 0, resume_key = 0, ntwrk_buf_start = 0x0, srch_entries_start = 0x0, last_entry = 0x0,
>     presume_name = 0x0, resume_name_len = 0, endOfSearch = false, emptyDir = false, unicode = false, smallBuf = false}}
> 

Luca...thanks for the bug report and analysis. I've been able to
reproduce this too against 3.3.2. The attached patch reverts the open
with lookup intent functionality that was added at the beginning of the
2.6.30 cycle. This fixes the problem for me.

Steve and Shirish...given that we've already seen a couple of
regressions from these patches, I'd like to see them reverted until
they have been better tested.

-- 
Jeff Layton <jlayton@redhat.com>

[-- Attachment #2: 0001--CIFS-Fix-double-list-addition-in-cifs-posix-open-c.patch --]
[-- Type: text/x-patch, Size: 3580 bytes --]

>From 90e4ee5d311d4e0729daa676b1d7f754265b5874 Mon Sep 17 00:00:00 2001
From: Steve French <sfrench@us.ibm.com>
Date: Fri, 8 May 2009 03:04:30 +0000
Subject: [PATCH] [CIFS] Fix double list addition in cifs posix open code

Remove adding open file entry twice to lists in the file
Do not fill file info twice in case of posix opens and creates

Signed-off-by: Shirish Pargaonkar <shirishp@us.ibm.com>
Signed-off-by: Steve French <sfrench@us.ibm.com>
---
 fs/cifs/dir.c  |   15 +++++++++------
 fs/cifs/file.c |   14 --------------
 2 files changed, 9 insertions(+), 20 deletions(-)

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 461750e..11431ed 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -281,6 +281,7 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
 	int create_options = CREATE_NOT_DIR;
 	int oplock = 0;
 	int oflags;
+	bool posix_create = false;
 	/*
 	 * BB below access is probably too much for mknod to request
 	 *    but we have to do query and setpathinfo so requesting
@@ -328,11 +329,13 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
 		   negotation.  EREMOTE indicates DFS junction, which is not
 		   handled in posix open */
 
-		if ((rc == 0) && (newinode == NULL))
-			goto cifs_create_get_file_info; /* query inode info */
-		else if (rc == 0) /* success, no need to query */
-			goto cifs_create_set_dentry;
-		else if ((rc != -EIO) && (rc != -EREMOTE) &&
+		if (rc == 0) {
+			posix_create = true;
+			if (newinode == NULL) /* query inode info */
+				goto cifs_create_get_file_info;
+			else /* success, no need to query */
+				goto cifs_create_set_dentry;
+		} else if ((rc != -EIO) && (rc != -EREMOTE) &&
 			 (rc != -EOPNOTSUPP)) /* path not found or net err */
 			goto cifs_create_out;
 		/* else fallthrough to retry, using older open call, this is
@@ -464,7 +467,7 @@ cifs_create_set_dentry:
 	if ((nd == NULL) || (!(nd->flags & LOOKUP_OPEN))) {
 		/* mknod case - do not leave file open */
 		CIFSSMBClose(xid, tcon, fileHandle);
-	} else if (newinode) {
+	} else if (!(posix_create) && (newinode)) {
 			cifs_fill_fileinfo(newinode, fileHandle,
 					cifs_sb->tcon, write_only);
 	}
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 50ca088..38c06f8 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -129,15 +129,12 @@ static inline int cifs_posix_open_inode_helper(struct inode *inode,
 			struct file *file, struct cifsInodeInfo *pCifsInode,
 			struct cifsFileInfo *pCifsFile, int oplock, u16 netfid)
 {
-	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
-/*	struct timespec temp; */   /* BB REMOVEME BB */
 
 	file->private_data = kmalloc(sizeof(struct cifsFileInfo), GFP_KERNEL);
 	if (file->private_data == NULL)
 		return -ENOMEM;
 	pCifsFile = cifs_init_private(file->private_data, inode, file, netfid);
 	write_lock(&GlobalSMBSeslock);
-	list_add(&pCifsFile->tlist, &cifs_sb->tcon->openFileList);
 
 	pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
 	if (pCifsInode == NULL) {
@@ -145,17 +142,6 @@ static inline int cifs_posix_open_inode_helper(struct inode *inode,
 		return -EINVAL;
 	}
 
-	/* want handles we can use to read with first
-	   in the list so we do not have to walk the
-	   list to search for one in write_begin */
-	if ((file->f_flags & O_ACCMODE) == O_WRONLY) {
-		list_add_tail(&pCifsFile->flist,
-			      &pCifsInode->openFileList);
-	} else {
-		list_add(&pCifsFile->flist,
-			 &pCifsInode->openFileList);
-	}
-
 	if (pCifsInode->clientCanCacheRead) {
 		/* we have the inode open somewhere else
 		   no need to discard cache data */
-- 
1.6.0.6

Re: [linux-cifs-client] Re: [2.6.30-rc6] cifs_close: NULL pointer dereference

[Jeff Layton]

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

On Sat, 16 May 2009 22:13:31 -0400
Jeff Layton <jlayton@redhat.com> wrote:

> On Sat, 16 May 2009 22:55:58 +0200
> Luca Tettamanti <kronos.it@gmail.com> wrote:
> 
> > On Sat, May 16, 2009 at 06:28:13PM +0200, Luca Tettamanti wrote:
> > > Hello,
> > > I just hit a NULL pointer dereference in cifs_close while accessing a file on a
> > > remote Samba shared directory.
> > [...]
> > > BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> > > IP: [<ffffffffa05f105c>] cifs_close+0x1c1/0x2e8 [cifs]
> > 
> > Ok, I've reproduced this on a machine with a serial port :)
> > 
> > 0xffffffff803574e0 in list_del (entry=0xffff8800ac184210)
> > at /home/kronos/src/linux-2.6.git/lib/list_debug.c:46
> > 46              WARN(entry->prev->next != entry,
> > (gdb) bt
> > #0  0xffffffff803574e0 in list_del (entry=0xffff8800ac184210)
> >     at /home/kronos/src/linux-2.6.git/lib/list_debug.c:46
> > #1  0xffffffff80319588 in cifs_close (inode=0xffff8800b8d5f088, file=0xffff8800ad8f6d00)
> >     at /home/kronos/src/linux-2.6.git/fs/cifs/file.c:670
> > #2  0xffffffff8029d95e in __fput (file=0xffff8800ad8f6d00)
> >     at /home/kronos/src/linux-2.6.git/fs/file_table.c:281
> > #3  0xffffffff8029da32 in fput (file=0xffff8800ac184210)
> >     at /home/kronos/src/linux-2.6.git/fs/file_table.c:227
> > #4  0xffffffff8029ad30 in filp_close (filp=0xffff8800ad8f6d00, id=0xffff880037a3e080)
> >     at /home/kronos/src/linux-2.6.git/fs/open.c:1108
> > #5  0xffffffff8029ade0 in sys_close (fd=0) 
> >     at /home/kronos/src/linux-2.6.git/fs/open.c:1137
> > #6  0xffffffff802271e4 in sysenter_dispatch ()
> >     at /home/kronos/src/linux-2.6.git/arch/x86/ia32/ia32entry.S:161
> > #7  0x0000000000000000 in ?? ()
> > 
> > (gdb) f 1
> > #1  0xffffffff80319588 in cifs_close (inode=0xffff8800b8d5f088, file=0xffff8800ad8f6d00)
> >     at /home/kronos/src/linux-2.6.git/fs/cifs/file.c:670
> > 670                     list_del(&pSMBFile->flist);
> > 
> > (gdb) p *&pSMBFile->flist
> > $2 = {next = 0x0, prev = 0x0}
> > 
> > (gdb) p *&pSMBFile->tlist
> > $5 = {next = 0x0, prev = 0x0}
> > 
> > So both flist and tlist were not initilized in cifs_open.
> > 
> > The content of the whole pSMBFile structure:
> > 
> > (gdb) p *pSMBFile
> > $6 = {tlist = {next = 0x0, prev = 0x0}, flist = {next = 0x0, prev = 0x0}, uid = 0, pid = 5322, netfid = 13880,
> >   pfile = 0xffff8800ad8f6d00, pInode = 0xffff8800b8d5f088, lock_mutex = {count = {counter = 1}, wait_lock = {raw_lock = {
> >         slock = 514}, magic = 3735899821, owner_cpu = 4294967295, owner = 0xffffffffffffffff, dep_map = {
> >         key = 0xffffffff806a5c98, class_cache = 0x0, name = 0xffffffff80537308 "&lock->wait_lock"}}, wait_list = {
> >       next = 0xffff8800ac184278, prev = 0xffff8800ac184278}, owner = 0x0, name = 0x0, magic = 0xffff8800ac184240, dep_map = {
> >       key = 0xffffffff80d7ff50, class_cache = 0xffffffff80835400, name = 0xffffffff8054cdfa "&private_data->lock_mutex"}},
> >   llist = {next = 0xffff8800ac1842b8, prev = 0xffff8800ac1842b8}, closePend = true, invalidHandle = false,
> >   messageMode = false, wrtPending = {counter = 0}, fh_mutex = {count = {counter = 1}, wait_lock = {raw_lock = {slock = 0},
> >       magic = 3735899821, owner_cpu = 4294967295, owner = 0xffffffffffffffff, dep_map = {key = 0xffffffff806a5c98,
> >         class_cache = 0x0, name = 0xffffffff80537308 "&lock->wait_lock"}}, wait_list = {next = 0xffff8800ac184308,
> >       prev = 0xffff8800ac184308}, owner = 0x0, name = 0x0, magic = 0xffff8800ac1842d0, dep_map = {key = 0xffffffff80d7ff58,
> >       class_cache = 0x0, name = 0xffffffff8054cde2 "&private_data->fh_mutex"}}, srch_inf = {index_of_last_entry = 0,
> >     entries_in_buffer = 0, info_level = 0, resume_key = 0, ntwrk_buf_start = 0x0, srch_entries_start = 0x0, last_entry = 0x0,
> >     presume_name = 0x0, resume_name_len = 0, endOfSearch = false, emptyDir = false, unicode = false, smallBuf = false}}
> > 
> 
> Luca...thanks for the bug report and analysis. I've been able to
> reproduce this too against 3.3.2. The attached patch reverts the open
> with lookup intent functionality that was added at the beginning of the
> 2.6.30 cycle. This fixes the problem for me.
> 
> Steve and Shirish...given that we've already seen a couple of
> regressions from these patches, I'd like to see them reverted until
> they have been better tested.
> 

Oops, wrong patch...this is the right one.

-- 
Jeff Layton <jlayton@redhat.com>

[-- Attachment #2: 0001-cifs-revert-patches-to-add-open-on-lookup.patch --]
[-- Type: text/x-patch, Size: 13983 bytes --]

>From 7ae9cf299fd28d23d3850ee4a4337e40139a63d7 Mon Sep 17 00:00:00 2001
From: Jeff Layton <jlayton@redhat.com>
Date: Sat, 16 May 2009 22:08:54 -0400
Subject: [PATCH] cifs: revert patches to add open on lookup

This patch reverts the patches for open on lookup functionality
that was added at the beginning of the 2.6.30 cycle. Specifically,
these commits:

a6ce4932fbdbcd8f8e8c6df76812014351c32892
bc8cd4390c9129fbd286b10fa99972dfb68cd069
88dd47fff4891545bfcfdf39146dde8380771766
90e4ee5d311d4e0729daa676b1d7f754265b5874

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/cifs/cifsglob.h |    2 +-
 fs/cifs/dir.c      |  154 ++++++++++++++++++----------------------------------
 fs/cifs/file.c     |   77 ++++++++++++++++----------
 3 files changed, 100 insertions(+), 133 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index a61ab77..f1f858a 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -350,7 +350,7 @@ struct cifsFileInfo {
 	bool invalidHandle:1;	/* file closed via session abend */
 	bool messageMode:1;	/* for pipes: message vs byte mode */
 	atomic_t wrtPending;   /* handle in use - defer close */
-	struct mutex fh_mutex; /* prevents reopen race after dead ses*/
+	struct semaphore fh_sem; /* prevents reopen race after dead ses*/
 	struct cifs_search_info srch_inf;
 };
 
diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 11431ed..e457e14 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -129,62 +129,12 @@ cifs_bp_rename_retry:
 	return full_path;
 }
 
-static void
-cifs_fill_fileinfo(struct inode *newinode, __u16 fileHandle,
-			struct cifsTconInfo *tcon, bool write_only)
-{
-	int oplock = 0;
-	struct cifsFileInfo *pCifsFile;
-	struct cifsInodeInfo *pCifsInode;
-
-	pCifsFile = kzalloc(sizeof(struct cifsFileInfo), GFP_KERNEL);
-
-	if (pCifsFile == NULL)
-		return;
-
-	if (oplockEnabled)
-		oplock = REQ_OPLOCK;
-
-	pCifsFile->netfid = fileHandle;
-	pCifsFile->pid = current->tgid;
-	pCifsFile->pInode = newinode;
-	pCifsFile->invalidHandle = false;
-	pCifsFile->closePend = false;
-	mutex_init(&pCifsFile->fh_mutex);
-	mutex_init(&pCifsFile->lock_mutex);
-	INIT_LIST_HEAD(&pCifsFile->llist);
-	atomic_set(&pCifsFile->wrtPending, 0);
-
-	/* set the following in open now
-			pCifsFile->pfile = file; */
-	write_lock(&GlobalSMBSeslock);
-	list_add(&pCifsFile->tlist, &tcon->openFileList);
-	pCifsInode = CIFS_I(newinode);
-	if (pCifsInode) {
-		/* if readable file instance put first in list*/
-		if (write_only)
-			list_add_tail(&pCifsFile->flist,
-				      &pCifsInode->openFileList);
-		else
-			list_add(&pCifsFile->flist, &pCifsInode->openFileList);
-
-		if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
-			pCifsInode->clientCanCacheAll = true;
-			pCifsInode->clientCanCacheRead = true;
-			cFYI(1, ("Exclusive Oplock inode %p", newinode));
-		} else if ((oplock & 0xF) == OPLOCK_READ)
-				pCifsInode->clientCanCacheRead = true;
-	}
-	write_unlock(&GlobalSMBSeslock);
-}
-
 int cifs_posix_open(char *full_path, struct inode **pinode,
 		    struct super_block *sb, int mode, int oflags,
 		    int *poplock, __u16 *pnetfid, int xid)
 {
 	int rc;
 	__u32 oplock;
-	bool write_only = false;
 	FILE_UNIX_BASIC_INFO *presp_data;
 	__u32 posix_flags = 0;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(sb);
@@ -222,8 +172,6 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
 	if (oflags & O_DIRECT)
 		posix_flags |= SMB_O_DIRECT;
 
-	if (!(oflags & FMODE_READ))
-		write_only = true;
 
 	rc = CIFSPOSIXCreate(xid, cifs_sb->tcon, posix_flags, mode,
 			pnetfid, presp_data, &oplock, full_path,
@@ -252,8 +200,6 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
 
 	posix_fill_in_inode(*pinode, presp_data, 1);
 
-	cifs_fill_fileinfo(*pinode, *pnetfid, cifs_sb->tcon, write_only);
-
 posix_open_ret:
 	kfree(presp_data);
 	return rc;
@@ -281,7 +227,6 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
 	int create_options = CREATE_NOT_DIR;
 	int oplock = 0;
 	int oflags;
-	bool posix_create = false;
 	/*
 	 * BB below access is probably too much for mknod to request
 	 *    but we have to do query and setpathinfo so requesting
@@ -296,6 +241,7 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
 	char *full_path = NULL;
 	FILE_ALL_INFO *buf = NULL;
 	struct inode *newinode = NULL;
+	struct cifsInodeInfo *pCifsInode;
 	int disposition = FILE_OVERWRITE_IF;
 	bool write_only = false;
 
@@ -329,13 +275,11 @@ cifs_create(struct inode *inode, struct dentry *direntry, int mode,
 		   negotation.  EREMOTE indicates DFS junction, which is not
 		   handled in posix open */
 
-		if (rc == 0) {
-			posix_create = true;
-			if (newinode == NULL) /* query inode info */
-				goto cifs_create_get_file_info;
-			else /* success, no need to query */
-				goto cifs_create_set_dentry;
-		} else if ((rc != -EIO) && (rc != -EREMOTE) &&
+		if ((rc == 0) && (newinode == NULL))
+			goto cifs_create_get_file_info; /* query inode info */
+		else if (rc == 0) /* success, no need to query */
+			goto cifs_create_set_dentry;
+		else if ((rc != -EIO) && (rc != -EREMOTE) &&
 			 (rc != -EOPNOTSUPP)) /* path not found or net err */
 			goto cifs_create_out;
 		/* else fallthrough to retry, using older open call, this is
@@ -467,9 +411,45 @@ cifs_create_set_dentry:
 	if ((nd == NULL) || (!(nd->flags & LOOKUP_OPEN))) {
 		/* mknod case - do not leave file open */
 		CIFSSMBClose(xid, tcon, fileHandle);
-	} else if (!(posix_create) && (newinode)) {
-			cifs_fill_fileinfo(newinode, fileHandle,
-					cifs_sb->tcon, write_only);
+	} else if (newinode) {
+		struct cifsFileInfo *pCifsFile =
+			kzalloc(sizeof(struct cifsFileInfo), GFP_KERNEL);
+
+		if (pCifsFile == NULL)
+			goto cifs_create_out;
+		pCifsFile->netfid = fileHandle;
+		pCifsFile->pid = current->tgid;
+		pCifsFile->pInode = newinode;
+		pCifsFile->invalidHandle = false;
+		pCifsFile->closePend     = false;
+		init_MUTEX(&pCifsFile->fh_sem);
+		mutex_init(&pCifsFile->lock_mutex);
+		INIT_LIST_HEAD(&pCifsFile->llist);
+		atomic_set(&pCifsFile->wrtPending, 0);
+
+		/* set the following in open now
+				pCifsFile->pfile = file; */
+		write_lock(&GlobalSMBSeslock);
+		list_add(&pCifsFile->tlist, &tcon->openFileList);
+		pCifsInode = CIFS_I(newinode);
+		if (pCifsInode) {
+			/* if readable file instance put first in list*/
+			if (write_only) {
+				list_add_tail(&pCifsFile->flist,
+					      &pCifsInode->openFileList);
+			} else {
+				list_add(&pCifsFile->flist,
+					 &pCifsInode->openFileList);
+			}
+			if ((oplock & 0xF) == OPLOCK_EXCLUSIVE) {
+				pCifsInode->clientCanCacheAll = true;
+				pCifsInode->clientCanCacheRead = true;
+				cFYI(1, ("Exclusive Oplock inode %p",
+					newinode));
+			} else if ((oplock & 0xF) == OPLOCK_READ)
+				pCifsInode->clientCanCacheRead = true;
+		}
+		write_unlock(&GlobalSMBSeslock);
 	}
 cifs_create_out:
 	kfree(buf);
@@ -602,21 +582,17 @@ int cifs_mknod(struct inode *inode, struct dentry *direntry, int mode,
 	return rc;
 }
 
+
 struct dentry *
 cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 	    struct nameidata *nd)
 {
 	int xid;
 	int rc = 0; /* to get around spurious gcc warning, set to zero here */
-	int oplock = 0;
-	int mode;
-	__u16 fileHandle = 0;
-	bool posix_open = false;
 	struct cifs_sb_info *cifs_sb;
 	struct cifsTconInfo *pTcon;
 	struct inode *newInode = NULL;
 	char *full_path = NULL;
-	struct file *filp;
 
 	xid = GetXid();
 
@@ -658,37 +634,12 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 	}
 	cFYI(1, ("Full path: %s inode = 0x%p", full_path, direntry->d_inode));
 
-	if (pTcon->unix_ext) {
-		if (!(nd->flags & (LOOKUP_PARENT | LOOKUP_DIRECTORY)) &&
-				(nd->flags & LOOKUP_OPEN)) {
-			if (!((nd->intent.open.flags & O_CREAT) &&
-					(nd->intent.open.flags & O_EXCL))) {
-				mode = nd->intent.open.create_mode &
-						~current_umask();
-				rc = cifs_posix_open(full_path, &newInode,
-					parent_dir_inode->i_sb, mode,
-					nd->intent.open.flags, &oplock,
-					&fileHandle, xid);
-				/*
-				 * This code works around a bug in
-				 * samba posix open in samba versions 3.3.1
-				 * and earlier where create works
-				 * but open fails with invalid parameter.
-				 * If either of these error codes are
-				 * returned, follow the normal lookup.
-				 * Otherwise, the error during posix open
-				 * is handled.
-				 */
-				if ((rc != -EINVAL) && (rc != -EOPNOTSUPP))
-					posix_open = true;
-			}
-		}
-		if (!posix_open)
-			rc = cifs_get_inode_info_unix(&newInode, full_path,
-						parent_dir_inode->i_sb, xid);
-	} else
+	if (pTcon->unix_ext)
+		rc = cifs_get_inode_info_unix(&newInode, full_path,
+					      parent_dir_inode->i_sb, xid);
+	else
 		rc = cifs_get_inode_info(&newInode, full_path, NULL,
-				parent_dir_inode->i_sb, xid, NULL);
+					 parent_dir_inode->i_sb, xid, NULL);
 
 	if ((rc == 0) && (newInode != NULL)) {
 		if (pTcon->nocase)
@@ -696,8 +647,7 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 		else
 			direntry->d_op = &cifs_dentry_ops;
 		d_add(direntry, newInode);
-		if (posix_open)
-			filp = lookup_instantiate_filp(nd, direntry, NULL);
+
 		/* since paths are not looked up by component - the parent
 		   directories are presumed to be good here */
 		renew_parental_timestamps(direntry);
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index 38c06f8..dfd3e6c 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -46,7 +46,7 @@ static inline struct cifsFileInfo *cifs_init_private(
 	memset(private_data, 0, sizeof(struct cifsFileInfo));
 	private_data->netfid = netfid;
 	private_data->pid = current->tgid;
-	mutex_init(&private_data->fh_mutex);
+	init_MUTEX(&private_data->fh_sem);
 	mutex_init(&private_data->lock_mutex);
 	INIT_LIST_HEAD(&private_data->llist);
 	private_data->pfile = file; /* needed for writepage */
@@ -129,12 +129,15 @@ static inline int cifs_posix_open_inode_helper(struct inode *inode,
 			struct file *file, struct cifsInodeInfo *pCifsInode,
 			struct cifsFileInfo *pCifsFile, int oplock, u16 netfid)
 {
+	struct cifs_sb_info *cifs_sb = CIFS_SB(inode->i_sb);
+/*	struct timespec temp; */   /* BB REMOVEME BB */
 
 	file->private_data = kmalloc(sizeof(struct cifsFileInfo), GFP_KERNEL);
 	if (file->private_data == NULL)
 		return -ENOMEM;
 	pCifsFile = cifs_init_private(file->private_data, inode, file, netfid);
 	write_lock(&GlobalSMBSeslock);
+	list_add(&pCifsFile->tlist, &cifs_sb->tcon->openFileList);
 
 	pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
 	if (pCifsInode == NULL) {
@@ -142,6 +145,17 @@ static inline int cifs_posix_open_inode_helper(struct inode *inode,
 		return -EINVAL;
 	}
 
+	/* want handles we can use to read with first
+	   in the list so we do not have to walk the
+	   list to search for one in write_begin */
+	if ((file->f_flags & O_ACCMODE) == O_WRONLY) {
+		list_add_tail(&pCifsFile->flist,
+			      &pCifsInode->openFileList);
+	} else {
+		list_add(&pCifsFile->flist,
+			 &pCifsInode->openFileList);
+	}
+
 	if (pCifsInode->clientCanCacheRead) {
 		/* we have the inode open somewhere else
 		   no need to discard cache data */
@@ -270,32 +284,35 @@ int cifs_open(struct inode *inode, struct file *file)
 	cifs_sb = CIFS_SB(inode->i_sb);
 	tcon = cifs_sb->tcon;
 
-	/* search inode for this file and fill in file->private_data */
-	pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
-	read_lock(&GlobalSMBSeslock);
-	list_for_each(tmp, &pCifsInode->openFileList) {
-		pCifsFile = list_entry(tmp, struct cifsFileInfo,
-				       flist);
-		if ((pCifsFile->pfile == NULL) &&
-		    (pCifsFile->pid == current->tgid)) {
-			/* mode set in cifs_create */
-
-			/* needed for writepage */
-			pCifsFile->pfile = file;
-
-			file->private_data = pCifsFile;
-			break;
+	if (file->f_flags & O_CREAT) {
+		/* search inode for this file and fill in file->private_data */
+		pCifsInode = CIFS_I(file->f_path.dentry->d_inode);
+		read_lock(&GlobalSMBSeslock);
+		list_for_each(tmp, &pCifsInode->openFileList) {
+			pCifsFile = list_entry(tmp, struct cifsFileInfo,
+					       flist);
+			if ((pCifsFile->pfile == NULL) &&
+			    (pCifsFile->pid == current->tgid)) {
+				/* mode set in cifs_create */
+
+				/* needed for writepage */
+				pCifsFile->pfile = file;
+
+				file->private_data = pCifsFile;
+				break;
+			}
+		}
+		read_unlock(&GlobalSMBSeslock);
+		if (file->private_data != NULL) {
+			rc = 0;
+			FreeXid(xid);
+			return rc;
+		} else {
+			if (file->f_flags & O_EXCL)
+				cERROR(1, ("could not find file instance for "
+					   "new file %p", file));
 		}
 	}
-	read_unlock(&GlobalSMBSeslock);
-
-	if (file->private_data != NULL) {
-		rc = 0;
-		FreeXid(xid);
-		return rc;
-	} else if ((file->f_flags & O_CREAT) && (file->f_flags & O_EXCL))
-			cERROR(1, ("could not find file instance for "
-				   "new file %p", file));
 
 	full_path = build_path_from_dentry(file->f_path.dentry);
 	if (full_path == NULL) {
@@ -483,9 +500,9 @@ static int cifs_reopen_file(struct file *file, bool can_flush)
 		return -EBADF;
 
 	xid = GetXid();
-	mutex_unlock(&pCifsFile->fh_mutex);
+	down(&pCifsFile->fh_sem);
 	if (!pCifsFile->invalidHandle) {
-		mutex_lock(&pCifsFile->fh_mutex);
+		up(&pCifsFile->fh_sem);
 		FreeXid(xid);
 		return 0;
 	}
@@ -516,7 +533,7 @@ static int cifs_reopen_file(struct file *file, bool can_flush)
 	if (full_path == NULL) {
 		rc = -ENOMEM;
 reopen_error_exit:
-		mutex_lock(&pCifsFile->fh_mutex);
+		up(&pCifsFile->fh_sem);
 		FreeXid(xid);
 		return rc;
 	}
@@ -558,14 +575,14 @@ reopen_error_exit:
 			 cifs_sb->local_nls, cifs_sb->mnt_cifs_flags &
 				CIFS_MOUNT_MAP_SPECIAL_CHR);
 	if (rc) {
-		mutex_lock(&pCifsFile->fh_mutex);
+		up(&pCifsFile->fh_sem);
 		cFYI(1, ("cifs_open returned 0x%x", rc));
 		cFYI(1, ("oplock: %d", oplock));
 	} else {
 reopen_success:
 		pCifsFile->netfid = netfid;
 		pCifsFile->invalidHandle = false;
-		mutex_lock(&pCifsFile->fh_mutex);
+		up(&pCifsFile->fh_sem);
 		pCifsInode = CIFS_I(inode);
 		if (pCifsInode) {
 			if (can_flush) {
-- 
1.6.2.2

Re: [linux-cifs-client] Re: [2.6.30-rc6] cifs_close: NULL pointer dereference

[Steve French]

On Sat, May 16, 2009 at 9:13 PM, Jeff Layton <jlayton@redhat.com> wrote:
> On Sat, 16 May 2009 22:55:58 +0200
> Luca Tettamanti <kronos.it@gmail.com> wrote:
>
>> On Sat, May 16, 2009 at 06:28:13PM +0200, Luca Tettamanti wrote:
>> > Hello,
>> > I just hit a NULL pointer dereference in cifs_close while accessing a file on a
>> > remote Samba shared directory.
>> [...]
>> > BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
>> > IP: [<ffffffffa05f105c>] cifs_close+0x1c1/0x2e8 [cifs]
>>
>> Ok, I've reproduced this on a machine with a serial port :)
>>
>> 0xffffffff803574e0 in list_del (entry=0xffff8800ac184210)
>> at /home/kronos/src/linux-2.6.git/lib/list_debug.c:46
>> 46              WARN(entry->prev->next != entry,
>> (gdb) bt
>> #0  0xffffffff803574e0 in list_del (entry=0xffff8800ac184210)
>>     at /home/kronos/src/linux-2.6.git/lib/list_debug.c:46
>> #1  0xffffffff80319588 in cifs_close (inode=0xffff8800b8d5f088, file=0xffff8800ad8f6d00)
>>     at /home/kronos/src/linux-2.6.git/fs/cifs/file.c:670
>> #2  0xffffffff8029d95e in __fput (file=0xffff8800ad8f6d00)
>>     at /home/kronos/src/linux-2.6.git/fs/file_table.c:281
>> #3  0xffffffff8029da32 in fput (file=0xffff8800ac184210)
>>     at /home/kronos/src/linux-2.6.git/fs/file_table.c:227
>> #4  0xffffffff8029ad30 in filp_close (filp=0xffff8800ad8f6d00, id=0xffff880037a3e080)
>>     at /home/kronos/src/linux-2.6.git/fs/open.c:1108
>> #5  0xffffffff8029ade0 in sys_close (fd=0)
>>     at /home/kronos/src/linux-2.6.git/fs/open.c:1137
>> #6  0xffffffff802271e4 in sysenter_dispatch ()
>>     at /home/kronos/src/linux-2.6.git/arch/x86/ia32/ia32entry.S:161
>> #7  0x0000000000000000 in ?? ()
>>
>> (gdb) f 1
>> #1  0xffffffff80319588 in cifs_close (inode=0xffff8800b8d5f088, file=0xffff8800ad8f6d00)
>>     at /home/kronos/src/linux-2.6.git/fs/cifs/file.c:670
>> 670                     list_del(&pSMBFile->flist);
>>
>> (gdb) p *&pSMBFile->flist
>> $2 = {next = 0x0, prev = 0x0}
>>
>> (gdb) p *&pSMBFile->tlist
>> $5 = {next = 0x0, prev = 0x0}
>>
>> So both flist and tlist were not initilized in cifs_open.
>>
>> The content of the whole pSMBFile structure:
>>
>
> Luca...thanks for the bug report and analysis. I've been able to
> reproduce this too against 3.3.2.

After reproducing the problem, I just looked at this in more detail:
the flist and tlist look like they were initialized, but the problem
is instead that they are being deleted from the wrong cifs file
struct.

Tthe create adds them to the 1st pSMBFile, the close deletes them from
the 1st pSMBFile, the subsequent open added them to the 2nd pSMBFile,
the close deleted them from the 1st pSMBfile (which oopses).

This should be straightforward to fix, but I want to talk with Shirish about it.


-- 
Thanks,

Steve

Re: [linux-cifs-client] Re: [2.6.30-rc6] cifs_close: NULL pointer dereference

[Shirish Pargaonkar]

On Sat, May 16, 2009 at 9:13 PM, Jeff Layton <jlayton@redhat.com> wrote:
> On Sat, 16 May 2009 22:55:58 +0200
> Luca Tettamanti <kronos.it@gmail.com> wrote:
>
>> On Sat, May 16, 2009 at 06:28:13PM +0200, Luca Tettamanti wrote:
>> > Hello,
>> > I just hit a NULL pointer dereference in cifs_close while accessing a file on a
>> > remote Samba shared directory.
>> [...]
>> > BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
>> > IP: [<ffffffffa05f105c>] cifs_close+0x1c1/0x2e8 [cifs]
>>
>> Ok, I've reproduced this on a machine with a serial port :)
>>
>> 0xffffffff803574e0 in list_del (entry=0xffff8800ac184210)
>> at /home/kronos/src/linux-2.6.git/lib/list_debug.c:46
>> 46              WARN(entry->prev->next != entry,
>> (gdb) bt
>> #0  0xffffffff803574e0 in list_del (entry=0xffff8800ac184210)
>>     at /home/kronos/src/linux-2.6.git/lib/list_debug.c:46
>> #1  0xffffffff80319588 in cifs_close (inode=0xffff8800b8d5f088, file=0xffff8800ad8f6d00)
>>     at /home/kronos/src/linux-2.6.git/fs/cifs/file.c:670
>> #2  0xffffffff8029d95e in __fput (file=0xffff8800ad8f6d00)
>>     at /home/kronos/src/linux-2.6.git/fs/file_table.c:281
>> #3  0xffffffff8029da32 in fput (file=0xffff8800ac184210)
>>     at /home/kronos/src/linux-2.6.git/fs/file_table.c:227
>> #4  0xffffffff8029ad30 in filp_close (filp=0xffff8800ad8f6d00, id=0xffff880037a3e080)
>>     at /home/kronos/src/linux-2.6.git/fs/open.c:1108
>> #5  0xffffffff8029ade0 in sys_close (fd=0)
>>     at /home/kronos/src/linux-2.6.git/fs/open.c:1137
>> #6  0xffffffff802271e4 in sysenter_dispatch ()
>>     at /home/kronos/src/linux-2.6.git/arch/x86/ia32/ia32entry.S:161
>> #7  0x0000000000000000 in ?? ()
>>
>> (gdb) f 1
>> #1  0xffffffff80319588 in cifs_close (inode=0xffff8800b8d5f088, file=0xffff8800ad8f6d00)
>>     at /home/kronos/src/linux-2.6.git/fs/cifs/file.c:670
>> 670                     list_del(&pSMBFile->flist);
>>
>> (gdb) p *&pSMBFile->flist
>> $2 = {next = 0x0, prev = 0x0}
>>
>> (gdb) p *&pSMBFile->tlist
>> $5 = {next = 0x0, prev = 0x0}
>>
>> So both flist and tlist were not initilized in cifs_open.
>>
>> The content of the whole pSMBFile structure:
>>
>> (gdb) p *pSMBFile
>> $6 = {tlist = {next = 0x0, prev = 0x0}, flist = {next = 0x0, prev = 0x0}, uid = 0, pid = 5322, netfid = 13880,
>>   pfile = 0xffff8800ad8f6d00, pInode = 0xffff8800b8d5f088, lock_mutex = {count = {counter = 1}, wait_lock = {raw_lock = {
>>         slock = 514}, magic = 3735899821, owner_cpu = 4294967295, owner = 0xffffffffffffffff, dep_map = {
>>         key = 0xffffffff806a5c98, class_cache = 0x0, name = 0xffffffff80537308 "&lock->wait_lock"}}, wait_list = {
>>       next = 0xffff8800ac184278, prev = 0xffff8800ac184278}, owner = 0x0, name = 0x0, magic = 0xffff8800ac184240, dep_map = {
>>       key = 0xffffffff80d7ff50, class_cache = 0xffffffff80835400, name = 0xffffffff8054cdfa "&private_data->lock_mutex"}},
>>   llist = {next = 0xffff8800ac1842b8, prev = 0xffff8800ac1842b8}, closePend = true, invalidHandle = false,
>>   messageMode = false, wrtPending = {counter = 0}, fh_mutex = {count = {counter = 1}, wait_lock = {raw_lock = {slock = 0},
>>       magic = 3735899821, owner_cpu = 4294967295, owner = 0xffffffffffffffff, dep_map = {key = 0xffffffff806a5c98,
>>         class_cache = 0x0, name = 0xffffffff80537308 "&lock->wait_lock"}}, wait_list = {next = 0xffff8800ac184308,
>>       prev = 0xffff8800ac184308}, owner = 0x0, name = 0x0, magic = 0xffff8800ac1842d0, dep_map = {key = 0xffffffff80d7ff58,
>>       class_cache = 0x0, name = 0xffffffff8054cde2 "&private_data->fh_mutex"}}, srch_inf = {index_of_last_entry = 0,
>>     entries_in_buffer = 0, info_level = 0, resume_key = 0, ntwrk_buf_start = 0x0, srch_entries_start = 0x0, last_entry = 0x0,
>>     presume_name = 0x0, resume_name_len = 0, endOfSearch = false, emptyDir = false, unicode = false, smallBuf = false}}
>>
>
> Luca...thanks for the bug report and analysis. I've been able to
> reproduce this too against 3.3.2. The attached patch reverts the open
> with lookup intent functionality that was added at the beginning of the
> 2.6.30 cycle. This fixes the problem for me.
>
> Steve and Shirish...given that we've already seen a couple of
> regressions from these patches, I'd like to see them reverted until
> they have been better tested.
>
> --
> Jeff Layton <jlayton@redhat.com>
>

Does this happen inspite of the patch 90e4ee5d311d4e0729daa676b1d7f754265b5874

Re: [linux-cifs-client] Re: [2.6.30-rc6] cifs_close: NULL pointer dereference

[Jeff Layton]

On Sun, 17 May 2009 09:40:44 -0500
Shirish Pargaonkar <shirishpargaonkar@gmail.com> wrote:

> On Sat, May 16, 2009 at 9:13 PM, Jeff Layton <jlayton@redhat.com> wrote:
> > On Sat, 16 May 2009 22:55:58 +0200
> > Luca Tettamanti <kronos.it@gmail.com> wrote:
> >
> >> On Sat, May 16, 2009 at 06:28:13PM +0200, Luca Tettamanti wrote:
> >> > Hello,
> >> > I just hit a NULL pointer dereference in cifs_close while accessing a file on a
> >> > remote Samba shared directory.
> >> [...]
> >> > BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
> >> > IP: [<ffffffffa05f105c>] cifs_close+0x1c1/0x2e8 [cifs]
> >>
> >> Ok, I've reproduced this on a machine with a serial port :)
> >>
> >> 0xffffffff803574e0 in list_del (entry=0xffff8800ac184210)
> >> at /home/kronos/src/linux-2.6.git/lib/list_debug.c:46
> >> 46              WARN(entry->prev->next != entry,
> >> (gdb) bt
> >> #0  0xffffffff803574e0 in list_del (entry=0xffff8800ac184210)
> >>     at /home/kronos/src/linux-2.6.git/lib/list_debug.c:46
> >> #1  0xffffffff80319588 in cifs_close (inode=0xffff8800b8d5f088, file=0xffff8800ad8f6d00)
> >>     at /home/kronos/src/linux-2.6.git/fs/cifs/file.c:670
> >> #2  0xffffffff8029d95e in __fput (file=0xffff8800ad8f6d00)
> >>     at /home/kronos/src/linux-2.6.git/fs/file_table.c:281
> >> #3  0xffffffff8029da32 in fput (file=0xffff8800ac184210)
> >>     at /home/kronos/src/linux-2.6.git/fs/file_table.c:227
> >> #4  0xffffffff8029ad30 in filp_close (filp=0xffff8800ad8f6d00, id=0xffff880037a3e080)
> >>     at /home/kronos/src/linux-2.6.git/fs/open.c:1108
> >> #5  0xffffffff8029ade0 in sys_close (fd=0)
> >>     at /home/kronos/src/linux-2.6.git/fs/open.c:1137
> >> #6  0xffffffff802271e4 in sysenter_dispatch ()
> >>     at /home/kronos/src/linux-2.6.git/arch/x86/ia32/ia32entry.S:161
> >> #7  0x0000000000000000 in ?? ()
> >>
> >> (gdb) f 1
> >> #1  0xffffffff80319588 in cifs_close (inode=0xffff8800b8d5f088, file=0xffff8800ad8f6d00)
> >>     at /home/kronos/src/linux-2.6.git/fs/cifs/file.c:670
> >> 670                     list_del(&pSMBFile->flist);
> >>
> >> (gdb) p *&pSMBFile->flist
> >> $2 = {next = 0x0, prev = 0x0}
> >>
> >> (gdb) p *&pSMBFile->tlist
> >> $5 = {next = 0x0, prev = 0x0}
> >>
> >> So both flist and tlist were not initilized in cifs_open.
> >>
> >> The content of the whole pSMBFile structure:
> >>
> >> (gdb) p *pSMBFile
> >> $6 = {tlist = {next = 0x0, prev = 0x0}, flist = {next = 0x0, prev = 0x0}, uid = 0, pid = 5322, netfid = 13880,
> >>   pfile = 0xffff8800ad8f6d00, pInode = 0xffff8800b8d5f088, lock_mutex = {count = {counter = 1}, wait_lock = {raw_lock = {
> >>         slock = 514}, magic = 3735899821, owner_cpu = 4294967295, owner = 0xffffffffffffffff, dep_map = {
> >>         key = 0xffffffff806a5c98, class_cache = 0x0, name = 0xffffffff80537308 "&lock->wait_lock"}}, wait_list = {
> >>       next = 0xffff8800ac184278, prev = 0xffff8800ac184278}, owner = 0x0, name = 0x0, magic = 0xffff8800ac184240, dep_map = {
> >>       key = 0xffffffff80d7ff50, class_cache = 0xffffffff80835400, name = 0xffffffff8054cdfa "&private_data->lock_mutex"}},
> >>   llist = {next = 0xffff8800ac1842b8, prev = 0xffff8800ac1842b8}, closePend = true, invalidHandle = false,
> >>   messageMode = false, wrtPending = {counter = 0}, fh_mutex = {count = {counter = 1}, wait_lock = {raw_lock = {slock = 0},
> >>       magic = 3735899821, owner_cpu = 4294967295, owner = 0xffffffffffffffff, dep_map = {key = 0xffffffff806a5c98,
> >>         class_cache = 0x0, name = 0xffffffff80537308 "&lock->wait_lock"}}, wait_list = {next = 0xffff8800ac184308,
> >>       prev = 0xffff8800ac184308}, owner = 0x0, name = 0x0, magic = 0xffff8800ac1842d0, dep_map = {key = 0xffffffff80d7ff58,
> >>       class_cache = 0x0, name = 0xffffffff8054cde2 "&private_data->fh_mutex"}}, srch_inf = {index_of_last_entry = 0,
> >>     entries_in_buffer = 0, info_level = 0, resume_key = 0, ntwrk_buf_start = 0x0, srch_entries_start = 0x0, last_entry = 0x0,
> >>     presume_name = 0x0, resume_name_len = 0, endOfSearch = false, emptyDir = false, unicode = false, smallBuf = false}}
> >>
> >
> > Luca...thanks for the bug report and analysis. I've been able to
> > reproduce this too against 3.3.2. The attached patch reverts the open
> > with lookup intent functionality that was added at the beginning of the
> > 2.6.30 cycle. This fixes the problem for me.
> >
> > Steve and Shirish...given that we've already seen a couple of
> > regressions from these patches, I'd like to see them reverted until
> > they have been better tested.
> >
> > --
> > Jeff Layton <jlayton@redhat.com>
> >
> 
> Does this happen inspite of the patch 90e4ee5d311d4e0729daa676b1d7f754265b5874

Yes. It's trivial to reproduce against a samba 3.3.2 server.

-- 
Jeff Layton <jlayton@redhat.com>

Re: [linux-cifs-client] Re: [2.6.30-rc6] cifs_close: NULL pointer dereference

[Luca Tettamanti]

On Sun, May 17, 2009 at 4:40 PM, Shirish Pargaonkar
<shirishpargaonkar@gmail.com> wrote:
> On Sat, May 16, 2009 at 9:13 PM, Jeff Layton <jlayton@redhat.com> wrote:
>> On Sat, 16 May 2009 22:55:58 +0200
>> Luca Tettamanti <kronos.it@gmail.com> wrote:
>>
>>> On Sat, May 16, 2009 at 06:28:13PM +0200, Luca Tettamanti wrote:
>>> > Hello,
>>> > I just hit a NULL pointer dereference in cifs_close while accessing a file on a
>>> > remote Samba shared directory.
>>> [...]
>>> > BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
>>> > IP: [<ffffffffa05f105c>] cifs_close+0x1c1/0x2e8 [cifs]
[...]
> Does this happen inspite of the patch 90e4ee5d311d4e0729daa676b1d7f754265b5874

Yes, I was running 2.6.30-rc6, which contains that commit.

Luca