broken cifsFileInfo handling in cifs_reopen_file codepath

[Jeff Layton-2]

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 <sfrench@...>
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 <sfrench@...>

...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.

Thanks,
--
Jeff Layton <jlayton@...>
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Steve French-2]

I thought you and I had crawled through this with Shirish early in the
summer, but I agree that the reopen path is hard to follow but it
should be easy to prove whether it leaks on reconnect.  I agree with
the general point though - we can't reallocate a file struct on
reconnect in the posix path and that seems straightforward

On Fri, Sep 18, 2009 at 12:06 PM, Jeff Layton <jlayton@...> 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 <sfrench@...>
> 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 <sfrench@...>
>
> ...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.
>
> Thanks,
> --
> Jeff Layton <jlayton@...>
>

--
Thanks,

Steve
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

On Fri, 18 Sep 2009 12:10:35 -0500
Steve French <smfrench@...> wrote:

> I thought you and I had crawled through this with Shirish early in the
> summer, but I agree that the reopen path is hard to follow but it
> should be easy to prove whether it leaks on reconnect.  I agree with
> the general point though - we can't reallocate a file struct on
> reconnect in the posix path and that seems straightforward
>

I don't recall walking through this particular chunk of code -- we did
discuss the open on lookup stuff, but that's only peripherally related
to this...

Note that this is actually a little worse than I originally stated.
Each cifsFileInfo struct holds an inode reference. So you're probably
leaking inode references in this codepath too.

The fix doesn't look too hard. You need to fix cifs_posix_open so that
it doesn't automatically create a cifsFileInfo itself. Either pass one
into it that's already allocated, or (even better) just have the
callers fill it out. It would also be good to consolidate
cifs_fill_fileinfo and cifs_init_private since they basically do the
same thing.

--
Jeff Layton <jlayton@...>
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Steve French-2]

On Fri, Sep 18, 2009 at 12:55 PM, Jeff Layton <jlayton@...> wrote:
> The fix doesn't look too hard. You need to fix cifs_posix_open so that
> it doesn't automatically create a cifsFileInfo itself. Either pass one
> into it that's already allocated, or (even better) just have the
> callers fill it out. It would also be good to consolidate
> cifs_fill_fileinfo and cifs_init_private since they basically do the
> same thing.

Yes - thinking along the same lines


--
Thanks,

Steve
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client

[Jeff Layton-2]

On Fri, 18 Sep 2009 13:06:33 -0400
Jeff Layton <jlayton@...> 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 <sfrench@...>
> 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 <sfrench@...>
>
> ...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 <jlayton@...>
_______________________________________________
linux-cifs-client mailing list
linux-cifs-client@...
https://lists.samba.org/mailman/listinfo/linux-cifs-client