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

[Jeff Layton]

 On Sat, 16 May 2009 22:55:58 +0200
Luca Tettamanti <kronos.it@xxxxxxxxx> 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@xxxxxxxxxx>