Design change for oplock/open code?

[Jeremy Allison]

On Thu, May 16, 2013 at 03:24:23PM +0200, Christian Ambach wrote:
> On 04/26/2013 11:46 PM, Jeremy Allison wrote:
> >I made 2 changes. One was to add a comment in
> >include/vfs.h saying the VFS interface number
> >wasn't yet being changed as we're not releasing
> >and noting the fields within the files_struct
> >that got deleted. The second was to up the debug
> >level on an error return from dbwrap_record_watch_recv()
> >from 1 to 5 as it can commonly get NT_STATUS_IO_TIMEOUT if
> >there really is a sharing violation.
> 
> That adjustment made it a bit harder for me to debug the issues
> I was experiencing when running bench.nbench and clustering=yes.
> 
> I was not able to see the NT_STATUS_ACCESS_DENIED at that spot with
> default loglevel, but I finally found the cause and I am proposing the
> attached patch to make it work again in clustering setups.
> Otherwise, bench.nbench will stall after a short while and smbd will
> spin in 100 %CPU until nbench hangs up because it cannot establish the
> message channel and so keeps retrying the open forever.
> 
> Cheers,
> Christian
> 

> From f2e1a8bb45f76300f87a3d6c3a052f6775ad01de Mon Sep 17 00:00:00 2001
> From: Christian Ambach <ambi at samba.org>
> Date: Thu, 16 May 2013 15:07:44 +0200
> Subject: [PATCH] s3:lib/ctdb_conn make sure we are root before connecting to
>  CTDB
> 
> CTDB socket is only reachable for root, make sure we are root when trying to connect to it
> 
> Signed-off-by: Christian Ambach <ambi at samba.org>
> ---
>  source3/lib/ctdb_conn.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/source3/lib/ctdb_conn.c b/source3/lib/ctdb_conn.c
> index a96615f..b4e144d 100644
> --- a/source3/lib/ctdb_conn.c
> +++ b/source3/lib/ctdb_conn.c
> @@ -81,10 +81,14 @@ struct tevent_req *ctdb_conn_init_send(TALLOC_CTX *mem_ctx,
>  	state->addr.sun_family = AF_UNIX;
>  	strncpy(state->addr.sun_path, sock, sizeof(state->addr.sun_path));
>  
> +	/* need to be root to connect to CTDB socket */
> +	become_root();
> +
>  	subreq = async_connect_send(state, ev, state->conn->fd,
>  				    (struct sockaddr *)&state->addr,
>  				    sizeof(state->addr));
>  	if (tevent_req_nomem(subreq, req)) {
> +		unbecome_root();
>  		return tevent_req_post(req, ev);
>  	}

This doesn't look safe. You're leaving us as root
across an async call.

Won't connecting to a UNIX domain socket be sync ?
Or at least doesn't only the the initial connect()
call need to be done as root ?

I'd be much happier with:

+	/* need to be root to connect to CTDB socket */
+	become_root();
+
 	subreq = async_connect_send(state, ev, state->conn->fd,
 				    (struct sockaddr *)&state->addr,
 				    sizeof(state->addr));

+	unbecome_root();
+

Would that work just as well ?

Jeremy.