Message-ID: <CAH2r5ms7Pr41haHJrnFoJLJMQMqP+pGm4ryuicbKch4y2-OV4w@mail.gmail.com>
Date:	Fri, 30 Sep 2011 17:20:27 -0500
From:	Steve French <smfrench@...il.com>
To:	Anton Altaparmakov <aia21@....ac.uk>
Cc:	Jeff Layton <jlayton@...ba.org>, Steve French <sfrench@...ba.org>,
	linux-cifs@...r.kernel.org, samba-technical@...ts.samba.org,
	LKML <linux-kernel@...r.kernel.org>,
	PWF Linux <pwf-linux@....cam.ac.uk>
Subject: Re: CIFS kernel module bug

On Fri, Sep 30, 2011 at 4:30 PM, Anton Altaparmakov <aia21@....ac.uk> wrote:
> Hi,
>
> On 30 Sep 2011, at 19:04, Jeff Layton wrote:
>> On Fri, 30 Sep 2011 14:58:58 +0100 Anton Altaparmakov <aia21@....ac.uk> wrote:
>>
>>> Hi,
>>>
>>> Looking at the current kernel (in Linus' repository on github) there is a silly logic bug in the cifs module in fs/cifs/cifsfs.c::cifs_llseek() there is this bit of code:
>>>
>>>      /*
>>>       * origin == SEEK_END || SEEK_DATA || SEEK_HOLE => we must revalidate
>>>       * the cached file length
>>>       */
>>>      if (origin != SEEK_SET || origin != SEEK_CUR) {
>>>
>>> The logical or should be a logical and, i.e. this should be:
>>>
>>>      if (origin != SEEK_SET && origin != SEEK_CUR) {
>>>
>>> As the code is at present that line is ALWAYS true because origin is ALWAYS either != SEEK_SET or != SEEK_CUR as if it equals one it cannot equal the other and vice versa…
>>>
>>> So at the moment it always does the revalidation instead of only for SEEK_END, SEEK_DATA, and SEEK_HOLE.
>>
>> Haha, good catch. Care to roll up a patch to fix that?
>
>
> Yes, I am happy to do that.
>
> Also a question for you: I am up to the neck in having to adapt the CIFS module as it doesn't work very well against the Novell Open Enterprise Server CIFS server (that is why I was looking at the CIFS code and noticed this logic bug in the first place).
>
> A bit of background: the home directories for our (i.e. University of Cambridge Computing Service) Linux distribution "MCS Linux" which runs on about a thousand workstations and a handfull of remote access servers used to be on Netware and were moved to OES Linux recently and the ncpfs kernel module really fell over in a heap when running against the OES NCP server so we had to switch to CIFS in a hurry and whilst it does not fall over in a heap like ncpfs does and we now have working reconnects when the home directory server is rebooted (home directories magically start working again after the reboot which is brilliant!) many applications do not work.
>
> I can only assume this is because the CIFS server is an abomination rather than that the CIFS modules is quite that badly broken…  Anyway, my question is: do you want patches that make the CIFS module work better with OES CIFS server or do you not care?
>
> For example lots of applications require working hard links so I had to port our Virtual Hard Link implementation from NCPfs to CIFS to get hard links working which fixed a lot of applications but then we started hitting real bugs.  A few examples:
>
> - llseek(SEEK_END) fails against the OES server with error -EBADF from the server because the SEEK_END causes file revalidation which ends up calling CIFSSMBQFileInfo() which then results in the -EBADF.  I wrote a patch to fall back to path based query via CIFSSMBQPathInfo() and if that fails via SMBQueryInformation() and now the llseek(SEEK_END) works thus applications like Seamonkey actually manage to launch rather than segfaulting.
>
> - mkdir() returns -EACCES when the target exists.  Again this is coming from the OES server.  I wrote a patch that calls cifs_get_inode_info() when CIFSSMBMkDir() returns -EACCES and unix extensions are not enabled on the superblock and if the result is success I rewrite the return code to -EEXIST which fixes the mkdir issue and that fixes loads of Gnome related applications.
>
> - The OES server behaves like NT4 in that it returns success from CIFSSMBSetPathInfo() but it actually ignores the call unless the file is open on the server.  But the OES server does not have CIFS_SES_NT4 in the session flags thus the work around in the CIFS module does not trigger and thus changing the access times or permissions does not work unless the file happens to be open which breaks applications like rsync.  I commented the CIFSSMBSetPathInfo() call completely in a patch so it falls back to the other code which makes it work for us.
>
> And finally the actual question: Are you interested in any of these patches or at least are you interested in fixing these issues in the standard CIFS module in which case I can send you my patches or at least give you detailed descriptions of what is failing and how?

Yes - we are strongly interested in making sure that the cifs module
is stable (always) against all servers (this is much harder than it
sounds) and works well against the major servers.   We travel to the
two major test events each year testing against the major servers (and
NAS).   For less functional servers, we are always interested in
stability fixes and as long as they are not too invasive, will
consider some workaround fixes to deal with server bugs if the patches
aren't high risk.

-- 
Thanks,

Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists