Konrad Rzeszutek Wilk
2012-Mar-22 18:38 UTC
Regression introduced by bfcfaa77bdf0f775263e906015982a608df01c76 (vfs: use ''unsigned long'' accesses for dcache name comparison and hashing)
Hey Linus,
I am not sure how to debug this but with v3.3 with just that git commit I
can''t
start an user space application called ''xenstored''. A bit of
strace showed me:
[edit, also with todays linus/master I get the same issue, and if I revert
your patch it works again]
(right is 3.3, left is with your patch)
write(3, "Checking store complete.\n", 25) = 25 write(3,
"Checking store complete.\n", 25) = 25
sendto(9, "<11>Mar 22 18:00:27 xenstored: C"..., 55, MSG_NOSI |
sendto(9, "<11>Mar 22 18:06:44 xenstored: C"..., 55, MSG_NOSI
open("/proc/xen/privcmd", O_RDWR) = 10
open("/proc/xen/privcmd", O_RDWR) = 10
fcntl(10, F_GETFD) = 0 fcntl(10, F_GETFD)
= 0
fcntl(10, F_SETFD, FD_CLOEXEC) = 0 fcntl(10, F_SETFD, FD_CLOEXEC)
= 0
open("/dev/xen/evtchn", O_RDWR) = 11
open("/dev/xen/evtchn", O_RDWR) = 11
open("/proc/xen/xsd_port", O_RDONLY) = -1 ENOENT (No such |
open("/proc/xen/xsd_port", O_RDONLY) = 12
write(2, "FATAL: ", 7FATAL: ) = 7 | read(12,
"25", 20) = 2
write(2, "Failed to initialize dom0 state:"..., 59Failed to i |
close(12) = 0
) = 59 | ioctl(11, EVIOCGVERSION, 0x7fff4e3ffc70) = 41
close(10) = 0 | write(3, "CREATE
connection 0xe2e7b0\n", 27) = 27
close(5) = 0 |
open("/proc/xen/xsd_kva", O_RDWR) = 12
close(4) = 0 | mmap(NULL, 4096,
PROT_READ|PROT_WRITE, MAP_SHARED, 12, 0) = 0
exit_group(1) = ? | close(12)
= 0
> ioctl(11, EVIOCGKEYCODE or EVIOCSKEYCODE, 0x7fff4e3ffc90) = 0
> ioctl(11, 0x44500, 0x7fff4e3ffc90) = 42
> rt_sigaction(SIGHUP, {0x402180, [HUP], SA_RESTORER|SA_RESTART
> select(12, [4 5 6 11], [], NULL, NULL) = 1 (in [11])
> read(11, ")\0\0\0", 4) = 4
> write(11, ")\0\0\0", 4) = 4
> select(12, [4 5 6 11], [], NULL, NULL <unfinished ...>
and if I ls /proc/xen (v3.3):
> ls -al /proc/xen
total 0
drwxr-xr-x 2 root root 0 Mar 22 18:05 .
dr-xr-xr-x 126 root root 0 Mar 22 18:05 ..
-r--r--r-- 1 root root 0 Mar 22 18:05 capabilities
-rw------- 1 root root 0 Mar 22 18:05 privcmd
-rw------- 1 root root 0 Mar 22 18:05 xenbus
-rw------- 1 root root 0 Mar 22 18:05 xsd_port
while v3.3 with your patch:
8:01:09 # 10 :/proc/xen/ > ls -al
ls: cannot access xsd_port: No such file or directory
total 0
drwxr-xr-x 2 root root 0 Mar 22 17:57 .
dr-xr-xr-x 126 root root 0 Mar 22 17:57 ..
-r--r--r-- 1 root root 0 Mar 22 17:57 capabilities
-rw------- 1 root root 0 Mar 22 17:57 privcmd
-rw------- 1 root root 0 Mar 22 17:57 xenbus
-rw------- 1 root root 0 Mar 22 17:57 xsd_kva
-????????? ? ? ? ? ? xsd_port
Looking at the code that sets up ''xsd_port'' it looks pretty
innocent
and similar to other drivers (ibmasm for example).
Also attached is the .config
Note: to actually bootup the latest with Xen there are some fixes required
to fix the regressions introduced:
http://git.kernel.org/?p=linux/kernel/git/konrad/xen.git;a=commit;h=106b44388d8f76373149c4ea144f717b6d4d9a6d
http://git.kernel.org/?p=linux/kernel/git/konrad/xen.git;a=commit;h=a759ceb7d1dfe38f4dda147e233aa53c8f477e2a
http://git.kernel.org/?p=linux/kernel/git/konrad/xen.git;a=commit;h=1b34ba936c1adff3020eec1e1834ffa4cf89802f
http://git.kernel.org/?p=linux/kernel/git/konrad/xen.git;a=commit;h=6ac72c4bc49a9afb02b8176b48be019dff0560cd
Eric Paris
2012-Mar-22 19:33 UTC
Re: Regression introduced by bfcfaa77bdf0f775263e906015982a608df01c76 (vfs: use ''unsigned long'' accesses for dcache name comparison and hashing)
No idea if it is related and haven''t bisected, but we are seeing similar ENOENT issues with things in selinuxfs. And not a single bit of SELinux code has changed since v3.3. So it''s certainly a VFS problem. This sounds likely. -Eric On Thu, Mar 22, 2012 at 2:38 PM, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:> Hey Linus, > > I am not sure how to debug this but with v3.3 with just that git commit I can''t > start an user space application called ''xenstored''. A bit of strace showed me: > [edit, also with todays linus/master I get the same issue, and if I revert > your patch it works again] > > (right is 3.3, left is with your patch) > write(3, "Checking store complete.\n", 25) = 25 write(3, "Checking store complete.\n", 25) = 25 > sendto(9, "<11>Mar 22 18:00:27 xenstored: C"..., 55, MSG_NOSI | sendto(9, "<11>Mar 22 18:06:44 xenstored: C"..., 55, MSG_NOSI > open("/proc/xen/privcmd", O_RDWR) = 10 open("/proc/xen/privcmd", O_RDWR) = 10 > fcntl(10, F_GETFD) = 0 fcntl(10, F_GETFD) = 0 > fcntl(10, F_SETFD, FD_CLOEXEC) = 0 fcntl(10, F_SETFD, FD_CLOEXEC) = 0 > open("/dev/xen/evtchn", O_RDWR) = 11 open("/dev/xen/evtchn", O_RDWR) = 11 > open("/proc/xen/xsd_port", O_RDONLY) = -1 ENOENT (No such | open("/proc/xen/xsd_port", O_RDONLY) = 12 > write(2, "FATAL: ", 7FATAL: ) = 7 | read(12, "25", 20) = 2 > write(2, "Failed to initialize dom0 state:"..., 59Failed to i | close(12) = 0 > ) = 59 | ioctl(11, EVIOCGVERSION, 0x7fff4e3ffc70) = 41 > close(10) = 0 | write(3, "CREATE connection 0xe2e7b0\n", 27) = 27 > close(5) = 0 | open("/proc/xen/xsd_kva", O_RDWR) = 12 > close(4) = 0 | mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, 12, 0) = 0 > exit_group(1) = ? | close(12) = 0 > > ioctl(11, EVIOCGKEYCODE or EVIOCSKEYCODE, 0x7fff4e3ffc90) = 0 > > ioctl(11, 0x44500, 0x7fff4e3ffc90) = 42 > > rt_sigaction(SIGHUP, {0x402180, [HUP], SA_RESTORER|SA_RESTART > > select(12, [4 5 6 11], [], NULL, NULL) = 1 (in [11]) > > read(11, ")\0\0\0", 4) = 4 > > write(11, ")\0\0\0", 4) = 4 > > select(12, [4 5 6 11], [], NULL, NULL <unfinished ...> > > and if I ls /proc/xen (v3.3): > >> ls -al /proc/xen > total 0 > drwxr-xr-x 2 root root 0 Mar 22 18:05 . > dr-xr-xr-x 126 root root 0 Mar 22 18:05 .. > -r--r--r-- 1 root root 0 Mar 22 18:05 capabilities > -rw------- 1 root root 0 Mar 22 18:05 privcmd > -rw------- 1 root root 0 Mar 22 18:05 xenbus > -rw------- 1 root root 0 Mar 22 18:05 xsd_port > > while v3.3 with your patch: > > > 8:01:09 # 10 :/proc/xen/ >> ls -al > ls: cannot access xsd_port: No such file or directory > total 0 > drwxr-xr-x 2 root root 0 Mar 22 17:57 . > dr-xr-xr-x 126 root root 0 Mar 22 17:57 .. > -r--r--r-- 1 root root 0 Mar 22 17:57 capabilities > -rw------- 1 root root 0 Mar 22 17:57 privcmd > -rw------- 1 root root 0 Mar 22 17:57 xenbus > -rw------- 1 root root 0 Mar 22 17:57 xsd_kva > -????????? ? ? ? ? ? xsd_port > > > Looking at the code that sets up ''xsd_port'' it looks pretty innocent > and similar to other drivers (ibmasm for example). > > Also attached is the .config > > Note: to actually bootup the latest with Xen there are some fixes required > to fix the regressions introduced: > http://git.kernel.org/?p=linux/kernel/git/konrad/xen.git;a=commit;h=106b44388d8f76373149c4ea144f717b6d4d9a6d > http://git.kernel.org/?p=linux/kernel/git/konrad/xen.git;a=commit;h=a759ceb7d1dfe38f4dda147e233aa53c8f477e2a > http://git.kernel.org/?p=linux/kernel/git/konrad/xen.git;a=commit;h=1b34ba936c1adff3020eec1e1834ffa4cf89802f > http://git.kernel.org/?p=linux/kernel/git/konrad/xen.git;a=commit;h=6ac72c4bc49a9afb02b8176b48be019dff0560cd
Eric Paris
2012-Mar-22 20:03 UTC
Re: Regression introduced by bfcfaa77bdf0f775263e906015982a608df01c76 (vfs: use ''unsigned long'' accesses for dcache name comparison and hashing)
Reverting this patch also fixed the spurious ENOENT problems (and related boot failures) from SELinux. Easy reproducer with SELinux enabled (although might I suggest permissive/enforcing=0 since the box can''t boot otherwise): compute_create system_u:system_r:kernel_t:s0 system_u:object_r:init_exec_t:s0 process If you get back system_u:object_r:init_exec_t:s0 it failed and you''ll see under strace the ENOENT stat() failure. If you get back system_u:object_r:init_t:s0 it means it worked... -Eric On Thu, Mar 22, 2012 at 3:33 PM, Eric Paris <eparis@parisplace.org> wrote:> No idea if it is related and haven''t bisected, but we are seeing > similar ENOENT issues with things in selinuxfs. And not a single bit > of SELinux code has changed since v3.3. So it''s certainly a VFS > problem. This sounds likely. > > -Eric > > On Thu, Mar 22, 2012 at 2:38 PM, Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com> wrote: >> Hey Linus, >> >> I am not sure how to debug this but with v3.3 with just that git commit I can''t >> start an user space application called ''xenstored''. A bit of strace showed me: >> [edit, also with todays linus/master I get the same issue, and if I revert >> your patch it works again] >> >> (right is 3.3, left is with your patch) >> write(3, "Checking store complete.\n", 25) = 25 write(3, "Checking store complete.\n", 25) = 25 >> sendto(9, "<11>Mar 22 18:00:27 xenstored: C"..., 55, MSG_NOSI | sendto(9, "<11>Mar 22 18:06:44 xenstored: C"..., 55, MSG_NOSI >> open("/proc/xen/privcmd", O_RDWR) = 10 open("/proc/xen/privcmd", O_RDWR) = 10 >> fcntl(10, F_GETFD) = 0 fcntl(10, F_GETFD) = 0 >> fcntl(10, F_SETFD, FD_CLOEXEC) = 0 fcntl(10, F_SETFD, FD_CLOEXEC) = 0 >> open("/dev/xen/evtchn", O_RDWR) = 11 open("/dev/xen/evtchn", O_RDWR) = 11 >> open("/proc/xen/xsd_port", O_RDONLY) = -1 ENOENT (No such | open("/proc/xen/xsd_port", O_RDONLY) = 12 >> write(2, "FATAL: ", 7FATAL: ) = 7 | read(12, "25", 20) = 2 >> write(2, "Failed to initialize dom0 state:"..., 59Failed to i | close(12) = 0 >> ) = 59 | ioctl(11, EVIOCGVERSION, 0x7fff4e3ffc70) = 41 >> close(10) = 0 | write(3, "CREATE connection 0xe2e7b0\n", 27) = 27 >> close(5) = 0 | open("/proc/xen/xsd_kva", O_RDWR) = 12 >> close(4) = 0 | mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_SHARED, 12, 0) = 0 >> exit_group(1) = ? | close(12) = 0 >> > ioctl(11, EVIOCGKEYCODE or EVIOCSKEYCODE, 0x7fff4e3ffc90) = 0 >> > ioctl(11, 0x44500, 0x7fff4e3ffc90) = 42 >> > rt_sigaction(SIGHUP, {0x402180, [HUP], SA_RESTORER|SA_RESTART >> > select(12, [4 5 6 11], [], NULL, NULL) = 1 (in [11]) >> > read(11, ")\0\0\0", 4) = 4 >> > write(11, ")\0\0\0", 4) = 4 >> > select(12, [4 5 6 11], [], NULL, NULL <unfinished ...> >> >> and if I ls /proc/xen (v3.3): >> >>> ls -al /proc/xen >> total 0 >> drwxr-xr-x 2 root root 0 Mar 22 18:05 . >> dr-xr-xr-x 126 root root 0 Mar 22 18:05 .. >> -r--r--r-- 1 root root 0 Mar 22 18:05 capabilities >> -rw------- 1 root root 0 Mar 22 18:05 privcmd >> -rw------- 1 root root 0 Mar 22 18:05 xenbus >> -rw------- 1 root root 0 Mar 22 18:05 xsd_port >> >> while v3.3 with your patch: >> >> >> 8:01:09 # 10 :/proc/xen/ >>> ls -al >> ls: cannot access xsd_port: No such file or directory >> total 0 >> drwxr-xr-x 2 root root 0 Mar 22 17:57 . >> dr-xr-xr-x 126 root root 0 Mar 22 17:57 .. >> -r--r--r-- 1 root root 0 Mar 22 17:57 capabilities >> -rw------- 1 root root 0 Mar 22 17:57 privcmd >> -rw------- 1 root root 0 Mar 22 17:57 xenbus >> -rw------- 1 root root 0 Mar 22 17:57 xsd_kva >> -????????? ? ? ? ? ? xsd_port >> >> >> Looking at the code that sets up ''xsd_port'' it looks pretty innocent >> and similar to other drivers (ibmasm for example). >> >> Also attached is the .config >> >> Note: to actually bootup the latest with Xen there are some fixes required >> to fix the regressions introduced: >> http://git.kernel.org/?p=linux/kernel/git/konrad/xen.git;a=commit;h=106b44388d8f76373149c4ea144f717b6d4d9a6d >> http://git.kernel.org/?p=linux/kernel/git/konrad/xen.git;a=commit;h=a759ceb7d1dfe38f4dda147e233aa53c8f477e2a >> http://git.kernel.org/?p=linux/kernel/git/konrad/xen.git;a=commit;h=1b34ba936c1adff3020eec1e1834ffa4cf89802f >> http://git.kernel.org/?p=linux/kernel/git/konrad/xen.git;a=commit;h=6ac72c4bc49a9afb02b8176b48be019dff0560cd
Al Viro
2012-Mar-22 20:09 UTC
Re: Regression introduced by bfcfaa77bdf0f775263e906015982a608df01c76 (vfs: use ''unsigned long'' accesses for dcache name comparison and hashing)
On Thu, Mar 22, 2012 at 02:38:46PM -0400, Konrad Rzeszutek Wilk wrote:> while v3.3 with your patch: > > > 8:01:09 # 10 :/proc/xen/ > > ls -al > ls: cannot access xsd_port: No such file or directory > total 0 > drwxr-xr-x 2 root root 0 Mar 22 17:57 . > dr-xr-xr-x 126 root root 0 Mar 22 17:57 .. > -r--r--r-- 1 root root 0 Mar 22 17:57 capabilities > -rw------- 1 root root 0 Mar 22 17:57 privcmd > -rw------- 1 root root 0 Mar 22 17:57 xenbus > -rw------- 1 root root 0 Mar 22 17:57 xsd_kva > -????????? ? ? ? ? ? xsd_port > > > Looking at the code that sets up ''xsd_port'' it looks pretty innocent > and similar to other drivers (ibmasm for example).Interesting... that''s exactly 8 characters. Oh, I see - hash_name() gets an extra multiplication by 9 in this case. Look: full_name_hash() will handle the first word, decrement len by 8, set hash to <first word> and bugger off on !len. hash_name(), OTOH, will go through the loops once, with hash and a both 0. hash stays 0, a becomes <first word>. No NUL or / in it, so in we go again; hash becomes a * 9, i.e. <first word> * 9. a becomes the second word, with mask != 0. And we are out of the loop, and proceed to add nothing to hash (the name is over at that point). As the result, we get hash mismatch for names that are 8 bytes long or multiple thereof.
Al Viro
2012-Mar-22 20:10 UTC
Re: Regression introduced by bfcfaa77bdf0f775263e906015982a608df01c76 (vfs: use ''unsigned long'' accesses for dcache name comparison and hashing)
On Thu, Mar 22, 2012 at 04:03:45PM -0400, Eric Paris wrote:> Reverting this patch also fixed the spurious ENOENT problems (and > related boot failures) from SELinux. > > Easy reproducer with SELinux enabled (although might I suggest > permissive/enforcing=0 since the box can''t boot otherwise): > > compute_create system_u:system_r:kernel_t:s0 > system_u:object_r:init_exec_t:s0 process > > If you get back system_u:object_r:init_exec_t:s0 it failed and you''ll > see under strace the ENOENT stat() failure. > > If you get back system_u:object_r:init_t:s0 it means it worked...Unless I''m misreading that code, the problem should hit on names with length being a multiple of 8...
Al Viro
2012-Mar-22 20:24 UTC
Re: Regression introduced by bfcfaa77bdf0f775263e906015982a608df01c76 (vfs: use ''unsigned long'' accesses for dcache name comparison and hashing)
On Thu, Mar 22, 2012 at 08:09:19PM +0000, Al Viro wrote:> Interesting... that''s exactly 8 characters. Oh, I see - hash_name() gets > an extra multiplication by 9 in this case. Look: full_name_hash() will > handle the first word, decrement len by 8, set hash to <first word> and > bugger off on !len. hash_name(), OTOH, will go through the loops once, > with hash and a both 0. hash stays 0, a becomes <first word>. No NUL or > / in it, so in we go again; hash becomes a * 9, i.e. <first word> * 9. > a becomes the second word, with mask != 0. And we are out of the loop, > and proceed to add nothing to hash (the name is over at that point). As > the result, we get hash mismatch for names that are 8 bytes long or > multiple thereof.OK, full_name_hash()/hash_name() definitely have a mismatch and it''s on the names of length 8*n: trivial experiment shows that we have name hash_name full_name_hash a 61 61 ab 6261 6261 abc 636261 636261 abcd 64636261 64636261 abcdabc 64c6c4c2 64c6c4c2 abcdabcd efcead5 c8c6c4c2 abcdabcd9 efceb0e efceb0e Linus, which way do you prefer to shift it? Should hash_name() change to match full_name_hash() or should it be the other way round? What happens is that you get multiplication by 9 and adding 0 in the former, after having added the last full word. In the latter we add the last full word, see that there''s nothing left and bugger off.
Al Viro
2012-Mar-22 20:36 UTC
Re: Regression introduced by bfcfaa77bdf0f775263e906015982a608df01c76 (vfs: use ''unsigned long'' accesses for dcache name comparison and hashing)
On Thu, Mar 22, 2012 at 08:24:45PM +0000, Al Viro wrote:> > OK, full_name_hash()/hash_name() definitely have a mismatch and it''s on the > names of length 8*n: trivial experiment shows that we have > name hash_name full_name_hash > a 61 61 > ab 6261 6261 > abc 636261 636261 > abcd 64636261 64636261 > abcdabc 64c6c4c2 64c6c4c2 > abcdabcd efcead5 c8c6c4c2 > abcdabcd9 efceb0e efceb0e > > Linus, which way do you prefer to shift it? Should hash_name() change to > match full_name_hash() or should it be the other way round? > > What happens is that you get multiplication by 9 and adding 0 in the former, > after having added the last full word. In the latter we add the last full > word, see that there''s nothing left and bugger off.Guys, could you check if this fixes it? diff --git a/fs/namei.c b/fs/namei.c index 13e6a1f..7451d6f8 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -1439,10 +1439,10 @@ unsigned int full_name_hash(const unsigned char *name, unsigned int len) for (;;) { a = *(unsigned long *)name; - hash *= 9; if (len < sizeof(unsigned long)) break; hash += a; + hash *= 9; name += sizeof(unsigned long); len -= sizeof(unsigned long); if (!len)
Linus Torvalds
2012-Mar-22 20:38 UTC
Re: Regression introduced by bfcfaa77bdf0f775263e906015982a608df01c76 (vfs: use ''unsigned long'' accesses for dcache name comparison and hashing)
On Thu, Mar 22, 2012 at 1:24 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:> > OK, full_name_hash()/hash_name() definitely have a mismatch and it''s on the > names of length 8*n: trivial experiment shows that we have > name hash_name full_name_hashGood catch, guys. Ugh. And I never noticed despite having run this code on my machines for several weeks, because I don''t think I have anything that uses the "full_name_hash()" function. And it looked so obviously the same.> Linus, which way do you prefer to shift it? Should hash_name() change to > match full_name_hash() or should it be the other way round? > > What happens is that you get multiplication by 9 and adding 0 in the former, > after having added the last full word. In the latter we add the last full > word, see that there''s nothing left and bugger off.Yes. I think we should make things match "hash_name()", because that''s the one that is critical and we want to really generate good code for. I think you can just move the "*=9" down in full_name_hash(), so that we always "pre-multiply" the hash for the next round. But I''ll have to double-check my logic. Linus
Linus Torvalds
2012-Mar-22 20:42 UTC
Re: Regression introduced by bfcfaa77bdf0f775263e906015982a608df01c76 (vfs: use ''unsigned long'' accesses for dcache name comparison and hashing)
On Thu, Mar 22, 2012 at 1:36 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:> > Guys, could you check if this fixes it?Yup, that''s the patch I was thinking of too. But I didn''t test it. And considering that apparently I don''t have anything that ever triggers this, I guess I should write some user-space harness thing for this. I actually wrote some *other* user-space testing functions (checking that the hash values we generated were good), but I never tested the "unimportant and totally obvious" full_name_hash() function. Btw, about test harnesses: I do have a patch to the kernel that exposes the dentry hash chains through /proc. Interesting to anybody else? I decided that I didn''t want to try to simulate the dentry hashing in user space, since part of the final hash function is the ''parent'' dentry pointer itself (the name hash part is user-spaceable, but I wanted to see the distribution of those parent bits too). Linus
Al Viro
2012-Mar-22 20:44 UTC
Re: Regression introduced by bfcfaa77bdf0f775263e906015982a608df01c76 (vfs: use ''unsigned long'' accesses for dcache name comparison and hashing)
On Thu, Mar 22, 2012 at 01:38:28PM -0700, Linus Torvalds wrote:> On Thu, Mar 22, 2012 at 1:24 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > OK, full_name_hash()/hash_name() definitely have a mismatch and it''s on the > > names of length 8*n: trivial experiment shows that we have > > name hash_name full_name_hash > > Good catch, guys. > > Ugh. And I never noticed despite having run this code on my machines > for several weeks, because I don''t think I have anything that uses the > "full_name_hash()" function. And it looked so obviously the same. > > > Linus, which way do you prefer to shift it? ?Should hash_name() change to > > match full_name_hash() or should it be the other way round? > > > > What happens is that you get multiplication by 9 and adding 0 in the former, > > after having added the last full word. ?In the latter we add the last full > > word, see that there''s nothing left and bugger off. > > Yes. I think we should make things match "hash_name()", because that''s > the one that is critical and we want to really generate good code for. > > I think you can just move the "*=9" down in full_name_hash(), so that > we always "pre-multiply" the hash for the next round. But I''ll have > to double-check my logic.See upthread for diff doing just that ;-) Let''s see if that fixes the crap guys are seeing... BTW, you have used full_name_hash(), just not on something 8 char long - devpts uses d_alloc_name(), but pty numbers tend to be less than ten millions...
Linus Torvalds
2012-Mar-22 20:52 UTC
Re: Regression introduced by bfcfaa77bdf0f775263e906015982a608df01c76 (vfs: use ''unsigned long'' accesses for dcache name comparison and hashing)
On Thu, Mar 22, 2012 at 1:44 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:> > See upthread for diff doing just that ;-) Let''s see if that fixes the > crap guys are seeing... BTW, you have used full_name_hash(), just not > on something 8 char long - devpts uses d_alloc_name(), but pty numbers > tend to be less than ten millions...Moving the "*=9" down to the "next" iteration does fix it for me in my user-space test I just whipped up. Not exhaustive, but at least the test confirms the thinking. But yeah, let''s verify that it also fixes the actual problem being reported. And you''re right, I don''t have ten million pty''s ;) Linus
Konrad Rzeszutek Wilk
2012-Mar-22 21:41 UTC
Re: Regression introduced by bfcfaa77bdf0f775263e906015982a608df01c76 (vfs: use ''unsigned long'' accesses for dcache name comparison and hashing)
On Thu, Mar 22, 2012 at 08:36:58PM +0000, Al Viro wrote:> On Thu, Mar 22, 2012 at 08:24:45PM +0000, Al Viro wrote: > > > > OK, full_name_hash()/hash_name() definitely have a mismatch and it''s on the > > names of length 8*n: trivial experiment shows that we have > > name hash_name full_name_hash > > a 61 61 > > ab 6261 6261 > > abc 636261 636261 > > abcd 64636261 64636261 > > abcdabc 64c6c4c2 64c6c4c2 > > abcdabcd efcead5 c8c6c4c2 > > abcdabcd9 efceb0e efceb0e > > > > Linus, which way do you prefer to shift it? Should hash_name() change to > > match full_name_hash() or should it be the other way round? > > > > What happens is that you get multiplication by 9 and adding 0 in the former, > > after having added the last full word. In the latter we add the last full > > word, see that there''s nothing left and bugger off. > > Guys, could you check if this fixes it?Works for me! Thanks for coming up with a patch so quickly. Please add Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Thanks.> > diff --git a/fs/namei.c b/fs/namei.c > index 13e6a1f..7451d6f8 100644 > --- a/fs/namei.c > +++ b/fs/namei.c > @@ -1439,10 +1439,10 @@ unsigned int full_name_hash(const unsigned char *name, unsigned int len) > > for (;;) { > a = *(unsigned long *)name; > - hash *= 9; > if (len < sizeof(unsigned long)) > break; > hash += a; > + hash *= 9; > name += sizeof(unsigned long); > len -= sizeof(unsigned long); > if (!len) > -- > 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/
Linus Torvalds
2012-Mar-22 21:54 UTC
Re: Regression introduced by bfcfaa77bdf0f775263e906015982a608df01c76 (vfs: use ''unsigned long'' accesses for dcache name comparison and hashing)
On Thu, Mar 22, 2012 at 2:41 PM, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:> > Works for me! Thanks for coming up with a patch so quickly. Please add > > Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>Goodie. Al, I can commit it as yours, but I''d want a sign-off and a commit message for that. Otherwise I''ll take credit.. Linus
Al Viro
2012-Mar-22 21:59 UTC
Re: Regression introduced by bfcfaa77bdf0f775263e906015982a608df01c76 (vfs: use ''unsigned long'' accesses for dcache name comparison and hashing)
On Thu, Mar 22, 2012 at 02:54:37PM -0700, Linus Torvalds wrote:> On Thu, Mar 22, 2012 at 2:41 PM, Konrad Rzeszutek Wilk > <konrad.wilk@oracle.com> wrote: > > > > Works for me! Thanks for coming up with a patch so quickly. Please add > > > > Reported-and-Tested-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > Goodie. Al, I can commit it as yours, but I''d want a sign-off and a > commit message for that. Otherwise I''ll take credit..Hmm... How about Fix full_name_hash() behaviour when length is a multiple of 8 We want it to match what hash_name() is doing, which means extra multiply by 9 in this case... Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- diff --git a/fs/namei.c b/fs/namei.c index 13e6a1f..7451d6f8 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -1439,10 +1439,10 @@ unsigned int full_name_hash(const unsigned char *name, unsigned int len) for (;;) { a = *(unsigned long *)name; - hash *= 9; if (len < sizeof(unsigned long)) break; hash += a; + hash *= 9; name += sizeof(unsigned long); len -= sizeof(unsigned long); if (!len)