Re: broken cifsFileInfo handling in cifs_reopen_file codepath

On Fri, 18 Sep 2009 13:06:33 -0400
Jeff Layton <[email protected]> wrote:

> I've found a pretty nasty set of bugs relating to how cifsFileInfo
> structs are handled when reopening a file. This patch added support for
> reopening a file after a reconnect using posix opens:
> 
> commit 7fc8f4e95bf9564045985bb206af8e28a5e4e28f
> Author: Steve French <[email protected]>
> Date:   Mon Feb 23 20:43:11 2009 +0000
> 
>     [CIFS] reopen file via newer posix open protocol operation if available
>     
>     If the network connection crashes, and we have to reopen files, preferentially
>     use the newer cifs posix open protocol operation if the server supports it.
>     
>     Signed-off-by: Steve French <[email protected]>
> 
> ...the problem here is that cifs_posix_open allocates a new
> cifsFileInfo struct on a successful open. That new struct is apparently
> ignored however. The private_data pointer for the VFS file struct is
> never updated to point to the new cifsFileInfo.
> 
> It seems like this will leak cifsFileInfo structs on reconnect at the
> very least. It may have other more harmful effects since you'll have
> dangling entries sitting on the "flist" for an inode.
> 
> I don't think it's sufficient to "put" the existing cifsFileInfo struct
> in favor of the new one since you may have other codepaths that have
> pointers and active references to the cifsFileInfo struct.
> 
> Steve, any chance you could fix this? I find this code a bit hard to
> follow and am not clear on how you intended it to work.
> 


It turns out that I'm mistaken here...cifs_reopen_file calls
cifs_posix_open with the inode pointer set to NULL. This short-circuits
the code that calls cifs_fill_fileinfo and prevents the leak I
described above.

I still wouldn't mind seeing this code consolidated some and have some
patches for that I'll send along after testing them a bit.

-- 
Jeff Layton <[email protected]>