[PATCH] Fix streams_xattr (and fruit) with kernel oplocks

[Richard Sharpe]

On Thu, Jul 27, 2017 at 11:15 AM, Ralph Böhme via samba-technical
<samba-technical at lists.samba.org> wrote:
> Hi folks,
>
> attached find a fix for bug 12791: when kernel oplocks are enabled any VFS
> module that does an open() on the underlying base file when processing a client
> "stream open" request is doomed to cause all sort of havoc as it may trigger a
> kernel oplock break which triggers panics and errors in the oplock break
> handlers and the async open handling.
>
> The basic idea of the fix is to *not* open() the underlying file for stream
> related ops if it's not really needed and can't be replaced with a path-name
> based equivalent. Luckily, in streams_xattr and fruit all calls can be replaced
> with path-name based equivalents -- problem solved.
>
> As the open() implementation in the VFS module has to return *something* that is
> not (int)-1 and is ideally unique per open (although that's probably not really
> required, but read on) I've opted to return a pipe fd, this satisfies both
> conditions.
>
> The patchset has been running at a large customer site for 4 weeks and so far
> the previous daily crashes are gone. It also just passed another private
> autobuild.

Reviewing the second patch set.

First patch: RB+
Second patch:

I have one cosmetic suggestion with respect to this:

--- a/source3/modules/vfs_streams_xattr.c
+++ b/source3/modules/vfs_streams_xattr.c
@@ -232,8 +232,6 @@ static int streams_xattr_fstat(vfs_handle_struct
*handle, files_struct *fsp,
        struct stream_io *io = (struct stream_io *)
                VFS_FETCH_FSP_EXTENSION(handle, fsp);

-       DEBUG(10, ("streams_xattr_fstat called for %d\n", fsp->fh->fd));
-
        if (io == NULL || fsp->base_fsp == NULL) {
                return SMB_VFS_NEXT_FSTAT(handle, fsp, sbuf);
        }
@@ -242,6 +240,8 @@ static int streams_xattr_fstat(vfs_handle_struct
*handle, files_struct *fsp,
                return -1;
        }

+       DBG_DEBUG("streams_xattr_fstat called for %s\n", fsp_str_dbg(io->fsp));
+
        /* Create an smb_filename with stream_name == NULL. */
        smb_fname_base = synthetic_smb_fname(talloc_tos(),
                                        io->base,

I agree with what you are doing, but I suggest you move the new
DBG_DEBUG to the original position and use fsp->fsp_name. This is
because there are two early exits from that function and if they are
taken we will have no idea that this function was called.

Otherwise: Second patch RB+

Third patch: RB+
Fourth patch: RB+

Fifth patch. I have a question. AFAIK, there is only one DACL/SD for
the whole files, so in streams_xattr_sys_acl_blob_get_fd, shouldn't it
be fsp->base_fsp->fsp_name here?

+       return SMB_VFS_NEXT_SYS_ACL_BLOB_GET_FILE(
+               handle, fsp->fsp_name, mem_ctx,
+               blob_description, blob);
+}

Otherwise Fifth paths: Would like someone else to review the stuff in
streams_xattr_fsync_send etc. It looks OK to me but my tevent FU is
weak.

Sixth patch (the one with the pipes): RB+
Seventh patch: RB+
Eighth patch: RB+
Ninth patch: RB+
Tenth patch: ad_get and ad_fget seem to have duplicated code. A
separate function might be useful although there are minor
differences.

Otherwise: Tenth patch: RB+

Eleventh patch: RB+
Christmas patch: RB+

-- 
Regards,
Richard Sharpe
(何以解憂？唯有杜康。--曹操)