[Regression] 2.6.38 ncpfs

[Regression] 2.6.38 ncpfs

[Bongani Hlope]

[-- Attachment #1: Type: Text/Plain, Size: 6593 bytes --]

Hi Nick

The following commit fb2d5b86aff355a27ebfc132d3c99f4a940cc3fe causes an oops ( 
when I enter or list a ncpfs mounted volume. The code that you replaced in 
dir.c checked that the d_inode pointer was not null before it did a mutex 
lock.

For some reason the first time readdir is called for a ncpfs mount point, that 
pointer seems to be null/invalid (check the rest of ncp_fill_cache, they 
always check before using that pointer.). The BUG_ON that checks if the mutex 
is locked causes an Invalid Opcode oops. The attached patch fixes it for me, 
I'm not good with the file system layer, worse ncpfs. Can you please apply it 
to make ncpfs usable again.

kernel BUG
at /home/bongani/development/c/kernel/linux-2.6.33/fs/dcache.c:2126!
invalid opcode: 0000 [#1] PREEMPT SMP 
last sysfs
file:
/sys/devices/pci0000:00/0000:00:1f.5/host2/target2:0:0/2:0:0:0/block/sdb/uevent
CPU 0 
Modules linked in: ncpfs nls_iso8859_1 nls_cp437 vfat fat uas
usb_storage fuse vmnet vmblock vsock vmci vmmon af_packet ipv6
snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_intel snd_hda_codec
snd_hwdep snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq
snd_seq_device snd_pcm_oss snd_pcm sg snd_timer snd_mixer_oss snd
iTCO_wdt iTCO_vendor_support serio_raw sr_mod soundcore snd_page_alloc
r8169 mii i2c_i801 radeon ttm drm_kms_helper drm i2c_algo_bit i2c_core
binfmt_misc cpufreq_ondemand cpufreq_conservative cpufreq_powersave
acpi_cpufreq freq_table mperf nvram evdev button ppdev parport_pc
parport processor ide_generic pata_jmicron ide_pci_generic ide_gd_mod
ide_core pata_acpi ata_generic ahci libahci ata_piix libata sd_mod
scsi_mod crc_t10dif ext4 jbd2 crc16 uhci_hcd ohci_hcd ehci_hcd usbhid
hid usbcore

Pid: 12199, comm: bash Not tainted 2.6.38 #1 Gigabyte Technology Co.,
Ltd. EG45M-UD2H/EG45M-UD2H
RIP: 0010:[<ffffffff8114d19c>]  [<ffffffff8114d19c>]
dentry_update_name_case+0x6c/0x80
RSP: 0018:ffff8801b5af99c8  EFLAGS: 00010246
RAX: 0000000000000001 RBX: ffff8801a81bd9c0 RCX: 0000000000000000
RDX: 0000000000000000 RSI: ffff8801b5af9a88 RDI: ffff8801a81bd9c0
RBP: ffff8801b5af99e8 R08: 0000000000000053 R09: ffff8801a81bd9f8
R10: 0000000000000000 R11: 0000000000000000 R12: ffff88010039c048
R13: ffff8801b5af9a88 R14: ffff8800b8d170c0 R15: ffff8801a81bd9c0
FS:  00007f74a48b9700(0000) GS:ffff8800bfc00000(0000)
knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
CR2: 00007f74a3ff2360 CR3: 000000011075c000 CR4: 00000000000406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process bash (pid: 12199, threadinfo ffff8801b5af8000, task
ffff8801bdf6e440)
Stack:
 ffff88010039c048 ffff8801b5af9e58 ffff88010039c048 ffff8801b5af9c08
 ffff8801b5af9bd8 ffffffffa05ef62f ffff8801b5af9a18 ffffffff8103e4f1
 ffff8801b5af9a38 ffffffff8114a030 ffff8801b5af9f38 0000000000000000
Call Trace:
 [<ffffffffa05ef62f>] ncp_fill_cache+0x1df/0x5a0 [ncpfs]
 [<ffffffff8103e4f1>] ? get_parent_ip+0x11/0x50
 [<ffffffff8114a030>] ? filldir+0x0/0xd0
 [<ffffffff8103e4f1>] ? get_parent_ip+0x11/0x50
 [<ffffffff8103ea7d>] ? sub_preempt_count+0x9d/0xd0
 [<ffffffff813b2e1c>] ? _raw_spin_unlock_irqrestore+0x2c/0x60
 [<ffffffffa05f78b0>] ? ncp_do_request+0x2b0/0x3c0 [ncpfs]
 [<ffffffff81074520>] ? autoremove_wake_function+0x0/0x40
 [<ffffffff81074520>] ? autoremove_wake_function+0x0/0x40
 [<ffffffffa05f7ad2>] ? ncp_request2+0x52/0x90 [ncpfs]
 [<ffffffffa05efad7>] ncp_read_volume_list+0xe7/0x120 [ncpfs]
 [<ffffffff8114a030>] ? filldir+0x0/0xd0
 [<ffffffff810e8bd6>] ? find_lock_page+0x26/0x80
 [<ffffffff81030000>] ? untrack_pfn_vma+0x0/0x70
 [<ffffffffa05f0150>] ncp_readdir+0x310/0x750 [ncpfs]
 [<ffffffff8114a030>] ? filldir+0x0/0xd0
 [<ffffffff8114a030>] ? filldir+0x0/0xd0
 [<ffffffff8114a268>] vfs_readdir+0xb8/0xe0
 [<ffffffff8114a3f5>] sys_getdents+0x85/0xf0
 [<ffffffff813b3815>] ? page_fault+0x25/0x30
 [<ffffffff81002fd2>] system_call_fastpath+0x16/0x1b
Code: 04 48 8b 7b 28 41 8b 55 04 49 8b 75 08 e8 cd 79 0b 00 ff 43 04 4c
89 e7 e8 22 5c 26 00 48 8b 5d e8 4c 8b 65 f0 4c 8b 6d f8 c9 c3 <0f> 0b
eb fe 0f 0b eb fe 66 66 66 2e 0f 1f 84 00 00 00 00 00 55 
RIP  [<ffffffff8114d19c>] dentry_update_name_case+0x6c/0x80
 RSP <ffff8801b5af99c8>

decodecode < ncpfs-oops2.txt 


Code: 04 48 8b 7b 28 41 8b 55 04 49 8b 75 08 e8 cd 79 0b 00 ff 43 04 4c 89 e7 
e8 22 5c 26 00 48 8b 5d e8 4c 8b 65 f0 4c 8b 6d f8 c9 c3 <0f> 0b eb fe 0f 0b 
eb fe 66 66 66 2e 0f 1f 84 00 00 00 00 00 55

All code

========

   0:   04 48                   add    $0x48,%al
   2:   8b 7b 28                mov    0x28(%rbx),%edi
   5:   41 8b 55 04             mov    0x4(%r13),%edx
   9:   49 8b 75 08             mov    0x8(%r13),%rsi
   d:   e8 cd 79 0b 00          callq  0xb79df
  12:   ff 43 04                incl   0x4(%rbx)
  15:   4c 89 e7                mov    %r12,%rdi
  18:   e8 22 5c 26 00          callq  0x265c3f
  1d:   48 8b 5d e8             mov    -0x18(%rbp),%rbx
  21:   4c 8b 65 f0             mov    -0x10(%rbp),%r12
  25:   4c 8b 6d f8             mov    -0x8(%rbp),%r13
  29:   c9                      leaveq 
  2a:   c3                      retq   
  2b:*  0f 0b                   ud2a        <-- trapping instruction
  2d:   eb fe                   jmp    0x2d
  2f:*  0f 0b                   ud2a        <-- trapping instruction
  31:   eb fe                   jmp    0x31
  33:   66 66 66 2e 0f 1f 84    data32 data32 nopw %cs:0x0(%rax,%rax,1)
  3a:   00 00 00 00 00 
  3f:   55                      push   %rbp

Code starting with the faulting instruction
===========================================

   0:   0f 0b                   ud2a   
   2:   eb fe                   jmp    0x2
   4:   0f 0b                   ud2a   
   6:   eb fe                   jmp    0x6
   8:   66 66 66 2e 0f 1f 84    data32 data32 nopw %cs:0x0(%rax,%rax,1)
   f:   00 00 00 00 00 
  14:   55                      push   %rbp

gdb gives me this:


(gdb) l *0xffffffff8114d19c

0xffffffff8114d19c is in dentry_update_name_case 
(/home/bongani/development/c/kernel/linux-2.6.33/fs/dcache.c:2126).

2121     * Parent inode i_mutex must be held over d_lookup and into this call 
(to
2122     * keep renames and concurrent inserts, and readdir(2) away).
2123     */
2124    void dentry_update_name_case(struct dentry *dentry, struct qstr *name)
2125    {
2126            BUG_ON(!mutex_is_locked(&dentry->d_inode->i_mutex));
2127            BUG_ON(dentry->d_name.len != name->len); /* d_lookup gives 
this */
2128
2129            spin_lock(&dentry->d_lock);
2130            write_seqcount_begin(&dentry->d_seq);

[-- Attachment #2: ncpfs.patch --]
[-- Type: text/x-patch, Size: 1135 bytes --]

--- linux-2.6.33/fs/dcache.c.org	2011-04-04 08:20:20.382107296 +0200
+++ linux-2.6.33/fs/dcache.c	2011-04-04 09:05:40.182805269 +0200
@@ -2123,7 +2123,6 @@
  */
 void dentry_update_name_case(struct dentry *dentry, struct qstr *name)
 {
-	BUG_ON(!mutex_is_locked(&dentry->d_inode->i_mutex));
 	BUG_ON(dentry->d_name.len != name->len); /* d_lookup gives this */
 
 	spin_lock(&dentry->d_lock);
--- linux-2.6.33/fs/ncpfs/dir.c.org	2011-04-04 08:16:21.812659616 +0200
+++ linux-2.6.33/fs/ncpfs/dir.c	2011-04-04 09:52:18.822549265 +0200
@@ -583,6 +583,7 @@
 	struct qstr qname;
 	int valid = 0;
 	int hashed = 0;
+	int wasLocked = 1;
 	ino_t ino = 0;
 	__u8 __name[NCP_MAXPATHLEN + 1];
 
@@ -620,7 +621,23 @@
 		 * server. Parent dir's i_mutex is locked because we're in
 		 * readdir.
 		 */
+
+		struct inode *inode = dentry->d_inode;
+
+		if(inode) {
+			if(!mutex_is_locked(&dentry->d_inode->i_mutex)) {
+				mutex_lock(&inode->i_mutex);
+				wasLocked = 0;
+			}
+		}
+		
 		dentry_update_name_case(newdent, &qname);
+
+		if(inode) {
+			if(!wasLocked) {
+				mutex_unlock(&inode->i_mutex);
+			}
+		}
 	}
 
 	if (!newdent->d_inode) {

Re: [Regression] 2.6.38 ncpfs

[Bongani Hlope]

[-- Attachment #1: Type: Text/Plain, Size: 7439 bytes --]

Hi Rafael, Greg and Linus

I reported a bug in the ncpfs , that causes an oops when you enter a recently 
mounted ncpfs volume. The bug was also reported by Dr. Bernd Feige in January. 
I have attached a patch that makes ncpfs usable again on the 2.6.38 kernel.

The bug is caused by commit fb2d5b86aff355a27ebfc132d3c99f4a940cc3fe, which 
was committed by Nick. Can you please either revert Nick's changes or include 
the attached patch (or a better version, because I'm not that clued up with 
ncpfs).

Regards
Bongani


On Tuesday 05 April 2011 18:58:20 Bongani Hlope wrote:
> Hi Nick
> 
> The following commit fb2d5b86aff355a27ebfc132d3c99f4a940cc3fe causes an
> oops ( when I enter or list a ncpfs mounted volume. The code that you
> replaced in dir.c checked that the d_inode pointer was not null before it
> did a mutex lock.
> 
> For some reason the first time readdir is called for a ncpfs mount point,
> that pointer seems to be null/invalid (check the rest of ncp_fill_cache,
> they always check before using that pointer.). The BUG_ON that checks if
> the mutex is locked causes an Invalid Opcode oops. The attached patch
> fixes it for me, I'm not good with the file system layer, worse ncpfs. Can
> you please apply it to make ncpfs usable again.
> 
> kernel BUG
> at /home/bongani/development/c/kernel/linux-2.6.33/fs/dcache.c:2126!
> invalid opcode: 0000 [#1] PREEMPT SMP
> last sysfs
> file:
> /sys/devices/pci0000:00/0000:00:1f.5/host2/target2:0:0/2:0:0:0/block/sdb/ue
> vent CPU 0
> Modules linked in: ncpfs nls_iso8859_1 nls_cp437 vfat fat uas
> usb_storage fuse vmnet vmblock vsock vmci vmmon af_packet ipv6
> snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_intel snd_hda_codec
> snd_hwdep snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq
> snd_seq_device snd_pcm_oss snd_pcm sg snd_timer snd_mixer_oss snd
> iTCO_wdt iTCO_vendor_support serio_raw sr_mod soundcore snd_page_alloc
> r8169 mii i2c_i801 radeon ttm drm_kms_helper drm i2c_algo_bit i2c_core
> binfmt_misc cpufreq_ondemand cpufreq_conservative cpufreq_powersave
> acpi_cpufreq freq_table mperf nvram evdev button ppdev parport_pc
> parport processor ide_generic pata_jmicron ide_pci_generic ide_gd_mod
> ide_core pata_acpi ata_generic ahci libahci ata_piix libata sd_mod
> scsi_mod crc_t10dif ext4 jbd2 crc16 uhci_hcd ohci_hcd ehci_hcd usbhid
> hid usbcore
> 
> Pid: 12199, comm: bash Not tainted 2.6.38 #1 Gigabyte Technology Co.,
> Ltd. EG45M-UD2H/EG45M-UD2H
> RIP: 0010:[<ffffffff8114d19c>]  [<ffffffff8114d19c>]
> dentry_update_name_case+0x6c/0x80
> RSP: 0018:ffff8801b5af99c8  EFLAGS: 00010246
> RAX: 0000000000000001 RBX: ffff8801a81bd9c0 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffff8801b5af9a88 RDI: ffff8801a81bd9c0
> RBP: ffff8801b5af99e8 R08: 0000000000000053 R09: ffff8801a81bd9f8
> R10: 0000000000000000 R11: 0000000000000000 R12: ffff88010039c048
> R13: ffff8801b5af9a88 R14: ffff8800b8d170c0 R15: ffff8801a81bd9c0
> FS:  00007f74a48b9700(0000) GS:ffff8800bfc00000(0000)
> knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
> CR2: 00007f74a3ff2360 CR3: 000000011075c000 CR4: 00000000000406f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
> Process bash (pid: 12199, threadinfo ffff8801b5af8000, task
> ffff8801bdf6e440)
> Stack:
>  ffff88010039c048 ffff8801b5af9e58 ffff88010039c048 ffff8801b5af9c08
>  ffff8801b5af9bd8 ffffffffa05ef62f ffff8801b5af9a18 ffffffff8103e4f1
>  ffff8801b5af9a38 ffffffff8114a030 ffff8801b5af9f38 0000000000000000
> Call Trace:
>  [<ffffffffa05ef62f>] ncp_fill_cache+0x1df/0x5a0 [ncpfs]
>  [<ffffffff8103e4f1>] ? get_parent_ip+0x11/0x50
>  [<ffffffff8114a030>] ? filldir+0x0/0xd0
>  [<ffffffff8103e4f1>] ? get_parent_ip+0x11/0x50
>  [<ffffffff8103ea7d>] ? sub_preempt_count+0x9d/0xd0
>  [<ffffffff813b2e1c>] ? _raw_spin_unlock_irqrestore+0x2c/0x60
>  [<ffffffffa05f78b0>] ? ncp_do_request+0x2b0/0x3c0 [ncpfs]
>  [<ffffffff81074520>] ? autoremove_wake_function+0x0/0x40
>  [<ffffffff81074520>] ? autoremove_wake_function+0x0/0x40
>  [<ffffffffa05f7ad2>] ? ncp_request2+0x52/0x90 [ncpfs]
>  [<ffffffffa05efad7>] ncp_read_volume_list+0xe7/0x120 [ncpfs]
>  [<ffffffff8114a030>] ? filldir+0x0/0xd0
>  [<ffffffff810e8bd6>] ? find_lock_page+0x26/0x80
>  [<ffffffff81030000>] ? untrack_pfn_vma+0x0/0x70
>  [<ffffffffa05f0150>] ncp_readdir+0x310/0x750 [ncpfs]
>  [<ffffffff8114a030>] ? filldir+0x0/0xd0
>  [<ffffffff8114a030>] ? filldir+0x0/0xd0
>  [<ffffffff8114a268>] vfs_readdir+0xb8/0xe0
>  [<ffffffff8114a3f5>] sys_getdents+0x85/0xf0
>  [<ffffffff813b3815>] ? page_fault+0x25/0x30
>  [<ffffffff81002fd2>] system_call_fastpath+0x16/0x1b
> Code: 04 48 8b 7b 28 41 8b 55 04 49 8b 75 08 e8 cd 79 0b 00 ff 43 04 4c
> 89 e7 e8 22 5c 26 00 48 8b 5d e8 4c 8b 65 f0 4c 8b 6d f8 c9 c3 <0f> 0b
> eb fe 0f 0b eb fe 66 66 66 2e 0f 1f 84 00 00 00 00 00 55
> RIP  [<ffffffff8114d19c>] dentry_update_name_case+0x6c/0x80
>  RSP <ffff8801b5af99c8>
> 
> decodecode < ncpfs-oops2.txt
> 
> 
> Code: 04 48 8b 7b 28 41 8b 55 04 49 8b 75 08 e8 cd 79 0b 00 ff 43 04 4c 89
> e7 e8 22 5c 26 00 48 8b 5d e8 4c 8b 65 f0 4c 8b 6d f8 c9 c3 <0f> 0b eb fe
> 0f 0b eb fe 66 66 66 2e 0f 1f 84 00 00 00 00 00 55
> 
> All code
> 
> ========
> 
>    0:   04 48                   add    $0x48,%al
>    2:   8b 7b 28                mov    0x28(%rbx),%edi
>    5:   41 8b 55 04             mov    0x4(%r13),%edx
>    9:   49 8b 75 08             mov    0x8(%r13),%rsi
>    d:   e8 cd 79 0b 00          callq  0xb79df
>   12:   ff 43 04                incl   0x4(%rbx)
>   15:   4c 89 e7                mov    %r12,%rdi
>   18:   e8 22 5c 26 00          callq  0x265c3f
>   1d:   48 8b 5d e8             mov    -0x18(%rbp),%rbx
>   21:   4c 8b 65 f0             mov    -0x10(%rbp),%r12
>   25:   4c 8b 6d f8             mov    -0x8(%rbp),%r13
>   29:   c9                      leaveq
>   2a:   c3                      retq
>   2b:*  0f 0b                   ud2a        <-- trapping instruction
>   2d:   eb fe                   jmp    0x2d
>   2f:*  0f 0b                   ud2a        <-- trapping instruction
>   31:   eb fe                   jmp    0x31
>   33:   66 66 66 2e 0f 1f 84    data32 data32 nopw %cs:0x0(%rax,%rax,1)
>   3a:   00 00 00 00 00
>   3f:   55                      push   %rbp
> 
> Code starting with the faulting instruction
> ===========================================
> 
>    0:   0f 0b                   ud2a
>    2:   eb fe                   jmp    0x2
>    4:   0f 0b                   ud2a
>    6:   eb fe                   jmp    0x6
>    8:   66 66 66 2e 0f 1f 84    data32 data32 nopw %cs:0x0(%rax,%rax,1)
>    f:   00 00 00 00 00
>   14:   55                      push   %rbp
> 
> gdb gives me this:
> 
> 
> (gdb) l *0xffffffff8114d19c
> 
> 0xffffffff8114d19c is in dentry_update_name_case
> (/home/bongani/development/c/kernel/linux-2.6.33/fs/dcache.c:2126).
> 
> 2121     * Parent inode i_mutex must be held over d_lookup and into this
> call (to
> 2122     * keep renames and concurrent inserts, and readdir(2) away).
> 2123     */
> 2124    void dentry_update_name_case(struct dentry *dentry, struct qstr
> *name) 2125    {
> 2126            BUG_ON(!mutex_is_locked(&dentry->d_inode->i_mutex));
> 2127            BUG_ON(dentry->d_name.len != name->len); /* d_lookup gives
> this */
> 2128
> 2129            spin_lock(&dentry->d_lock);
> 2130            write_seqcount_begin(&dentry->d_seq);

[-- Attachment #2: ncpfs.patch --]
[-- Type: text/x-patch, Size: 1135 bytes --]

--- linux-2.6.33/fs/dcache.c.org	2011-04-04 08:20:20.382107296 +0200
+++ linux-2.6.33/fs/dcache.c	2011-04-04 09:05:40.182805269 +0200
@@ -2123,7 +2123,6 @@
  */
 void dentry_update_name_case(struct dentry *dentry, struct qstr *name)
 {
-	BUG_ON(!mutex_is_locked(&dentry->d_inode->i_mutex));
 	BUG_ON(dentry->d_name.len != name->len); /* d_lookup gives this */
 
 	spin_lock(&dentry->d_lock);
--- linux-2.6.33/fs/ncpfs/dir.c.org	2011-04-04 08:16:21.812659616 +0200
+++ linux-2.6.33/fs/ncpfs/dir.c	2011-04-04 09:52:18.822549265 +0200
@@ -583,6 +583,7 @@
 	struct qstr qname;
 	int valid = 0;
 	int hashed = 0;
+	int wasLocked = 1;
 	ino_t ino = 0;
 	__u8 __name[NCP_MAXPATHLEN + 1];
 
@@ -620,7 +621,23 @@
 		 * server. Parent dir's i_mutex is locked because we're in
 		 * readdir.
 		 */
+
+		struct inode *inode = dentry->d_inode;
+
+		if(inode) {
+			if(!mutex_is_locked(&dentry->d_inode->i_mutex)) {
+				mutex_lock(&inode->i_mutex);
+				wasLocked = 0;
+			}
+		}
+		
 		dentry_update_name_case(newdent, &qname);
+
+		if(inode) {
+			if(!wasLocked) {
+				mutex_unlock(&inode->i_mutex);
+			}
+		}
 	}
 
 	if (!newdent->d_inode) {

Re: [Regression] 2.6.38 ncpfs

[Linus Torvalds]

On Thu, Apr 14, 2011 at 10:14 AM, Bongani Hlope <bonganilinux@mweb.co.za> wrote:
>
> The bug is caused by commit fb2d5b86aff355a27ebfc132d3c99f4a940cc3fe, which
> was committed by Nick. Can you please either revert Nick's changes or include
> the attached patch (or a better version, because I'm not that clued up with
> ncpfs).

Hmm. Your patch isn't correct. You can't just do

   if (!mutex_is_locked(&dentry->d_inode->i_mutex)) {
      mutex_lock(&inode->i_mutex);

because code like that makes no sense - maybe it was locked by
somebody _else_, and you would need to lock it.

That said, I do think that the

  BUG_ON(!mutex_is_locked(&dentry->d_inode->i_mutex));

is wrong, since even the comment above it talks about the _parent_
dentry, not the dentry that is actually being modified.

So there's clearly a bug somewhere, and the fix may be to just remove
that BUG_ON().

Added some more people to the cc.

                                   Linus

Re: [Regression] 2.6.38 ncpfs

[Bongani Hlope]

[-- Attachment #1: Type: Text/Plain, Size: 2422 bytes --]

On Thursday 14 April 2011 19:29:34 Linus Torvalds wrote:
> On Thu, Apr 14, 2011 at 10:14 AM, Bongani Hlope <bonganilinux@mweb.co.za> 
wrote:
> > The bug is caused by commit fb2d5b86aff355a27ebfc132d3c99f4a940cc3fe,
> > which was committed by Nick. Can you please either revert Nick's changes
> > or include the attached patch (or a better version, because I'm not that
> > clued up with ncpfs).
> 
> Hmm. Your patch isn't correct. You can't just do
> 
>    if (!mutex_is_locked(&dentry->d_inode->i_mutex)) {
>       mutex_lock(&inode->i_mutex);
> 
> because code like that makes no sense - maybe it was locked by
> somebody _else_, and you would need to lock it.
> 
> That said, I do think that the
> 
>   BUG_ON(!mutex_is_locked(&dentry->d_inode->i_mutex));
> 
> is wrong, since even the comment above it talks about the _parent_
> dentry, not the dentry that is actually being modified.
> 
> So there's clearly a bug somewhere, and the fix may be to just remove
> that BUG_ON().
> 
> Added some more people to the cc.
> 
>                                    Linus

Looking at the changes,  you are right. The BUG_ON() seems to be what is not 
suppose to be there. The attached patch only removes the BUG_ON(). I'll test 
on my work PC tomorrow and see if that also fixes the bug.

The previous code was:

               if (qname.len == newdent->d_name.len &&
                   memcmp(newdent->d_name.name, qname.name, newdent-
>d_name.len)) {
8<
                        */
                       if (inode)
                               mutex_lock(&inode->i_mutex);
                       spin_lock(&dcache_lock);
                       spin_lock(&newdent->d_lock);
                       memcpy((char *) newdent->d_name.name, qname.name,
                                                               newdent-
>d_name.len);
                       spin_unlock(&newdent->d_lock);
                       spin_unlock(&dcache_lock);
                       if (inode)
                               mutex_unlock(&inode->i_mutex);
               }

and it was replaced by:

       BUG_ON(!mutex_is_locked(&dentry->d_inode->i_mutex));
       BUG_ON(dentry->d_name.len != name->len); /* d_lookup gives this */

       spin_lock(&dcache_lock);
       spin_lock(&dentry->d_lock);
       memcpy((unsigned char *)dentry->d_name.name, name->name, name->len);
       spin_unlock(&dentry->d_lock);
       spin_unlock(&dcache_lock);

[-- Attachment #2: ncpfs.patch --]
[-- Type: text/x-patch, Size: 388 bytes --]

diff --git a/fs/dcache.c b/fs/dcache.c
index ad25c4c..39fa508 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2131,7 +2131,6 @@ EXPORT_SYMBOL(d_rehash);
  */
 void dentry_update_name_case(struct dentry *dentry, struct qstr *name)
 {
-	BUG_ON(!mutex_is_locked(&dentry->d_inode->i_mutex));
 	BUG_ON(dentry->d_name.len != name->len); /* d_lookup gives this */
 
 	spin_lock(&dentry->d_lock);

Re: [Regression] 2.6.38 ncpfs

[Linus Torvalds]

On Thu, Apr 14, 2011 at 1:17 PM, Bongani Hlope <bonganilinux@mweb.co.za> wrote:
>
> Looking at the changes,  you are right. The BUG_ON() seems to be what is not
> suppose to be there. The attached patch only removes the BUG_ON(). I'll test
> on my work PC tomorrow and see if that also fixes the bug.

You might also try to replace it with

    BUG_ON(!mutex_is_locked(&dentry->d_parent->d_inode->i_mutex))

ie add that "d_parent" there. Just for testing - I think the real fix
really is to remove it, but I'd personally be happier knowing that
_if_ it were to have that d_parent there, it would have worked.

It would also be interesting to hear if that name length could
possibly ever change, and we'd hit that test too. It looks like the
original code actually checked that the length was the same before
doing the overwrite.

ncpfs doesn't seem to be very actively maintained, I suspect Petr
isn't really using it any more.

                   Linus

                Linus

Re: [Regression] 2.6.38 ncpfs

[Dr. Bernd Feige]

Hi Bongani, Linus et al.

> > You might also try to replace it with
> > 
> >     BUG_ON(!mutex_is_locked(&dentry->d_parent->d_inode->i_mutex))
> > 
> > ie add that "d_parent" there. Just for testing - I think the real fix
> > really is to remove it, but I'd personally be happier knowing that
> > _if_ it were to have that d_parent there, it would have worked.
> 
> I've made that change and ncpfs works fine with it. I've attached a
> patch that changes the BUG_ON to look at the parent.

Seconded, checked it with the Novell servers in our clinic and it works
like a charm - even with the large directories for which I still have to
revert commit 3825bdb7ed920845961f32f364454bee5f469abb from 2.6.37.6
(https://lkml.org/lkml/2011/1/26/302). Thanks a lot!

Bernd

Re: [Regression] 2.6.38 ncpfs

[Bongani Hlope]

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

Hi Linus

On Thu, 2011-04-14 at 13:22 -0700, Linus Torvalds wrote:
> On Thu, Apr 14, 2011 at 1:17 PM, Bongani Hlope
<bonganilinux@mweb.co.za> wrote:
> >
> > Looking at the changes,  you are right. The BUG_ON() seems to be
what is not
> > suppose to be there. The attached patch only removes the BUG_ON().
I'll test
> > on my work PC tomorrow and see if that also fixes the bug.
> 
> You might also try to replace it with
> 
>     BUG_ON(!mutex_is_locked(&dentry->d_parent->d_inode->i_mutex))
> 
> ie add that "d_parent" there. Just for testing - I think the real fix
> really is to remove it, but I'd personally be happier knowing that
> _if_ it were to have that d_parent there, it would have worked.

I've made that change and ncpfs works fine with it. I've attached a
patch that changes the BUG_ON to look at the parent.

> 
> It would also be interesting to hear if that name length could
> possibly ever change, and we'd hit that test too. It looks like the
> original code actually checked that the length was the same before
> doing the overwrite.
> 
> ncpfs doesn't seem to be very actively maintained, I suspect Petr
> isn't really using it any more.
> 
>                    Linus
> 
>                 Linus
> --
> To unsubscribe from this list: send the line "unsubscribe
linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 



 
 
This e-mail and its attachments, if any, are subject to BankservAfrica's
e-mail disclaimer which is available on
http://www.bankservafrica.com/Contactus/EmailDisclaimer.aspx.
 
Please consider the environment before printing this e-mail!

[-- Attachment #2: ncpfs.patch --]
[-- Type: text/plain, Size: 468 bytes --]

--- linux-2.6.33/fs/dcache.c.org	2011-04-04 08:20:20.382107296 +0200
+++ linux-2.6.33/fs/dcache.c	2011-04-15 08:23:45.530530995 +0200
@@ -2123,7 +2123,7 @@
  */
 void dentry_update_name_case(struct dentry *dentry, struct qstr *name)
 {
-	BUG_ON(!mutex_is_locked(&dentry->d_inode->i_mutex));
+	BUG_ON(!mutex_is_locked(&dentry->d_parent->d_inode->i_mutex));
 	BUG_ON(dentry->d_name.len != name->len); /* d_lookup gives this */
 
 	spin_lock(&dentry->d_lock);