Design change for oplock/open code?
Volker Lendecke
Volker.Lendecke at SerNet.DE
Thu May 16 08:17:49 MDT 2013
On Thu, May 16, 2013 at 06:44:24AM -0700, Jeremy Allison wrote:
> 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 ?
This is async, right? The real connect(2) in theory might
happen a lot later.
What about the following patch with the obvoius follow-up?
Volker
--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
-------------- next part --------------
From cc3861bd2d7b53eaa05baa8f9e2c5e7be445d451 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 16 May 2013 16:11:54 +0200
Subject: [PATCH] lib: Add before/after hooks to async_connect
This will facilitiate [un]become_root for smbd to connect safely to ctdbd.
---
lib/async_req/async_sock.c | 35 +++++++++++++++++++++++++++++++----
lib/async_req/async_sock.h | 10 ++++++----
source3/lib/ctdb_conn.c | 2 +-
source3/lib/util_sock.c | 4 ++--
source3/libsmb/unexpected.c | 2 +-
source3/torture/wbc_async.c | 2 +-
6 files changed, 42 insertions(+), 13 deletions(-)
diff --git a/lib/async_req/async_sock.c b/lib/async_req/async_sock.c
index 9909bc6..59dde88 100644
--- a/lib/async_req/async_sock.c
+++ b/lib/async_req/async_sock.c
@@ -217,6 +217,10 @@ struct async_connect_state {
long old_sockflags;
socklen_t address_len;
struct sockaddr_storage address;
+
+ void (*before_connect)(void *private_data);
+ void (*after_connect)(void *private_data);
+ void *private_data;
};
static void async_connect_connected(struct tevent_context *ev,
@@ -236,10 +240,12 @@ static void async_connect_connected(struct tevent_context *ev,
* connect in an async state. This will be reset when the request is finished.
*/
-struct tevent_req *async_connect_send(TALLOC_CTX *mem_ctx,
- struct tevent_context *ev,
- int fd, const struct sockaddr *address,
- socklen_t address_len)
+struct tevent_req *async_connect_send(
+ TALLOC_CTX *mem_ctx, struct tevent_context *ev, int fd,
+ const struct sockaddr *address, socklen_t address_len,
+ void (*before_connect)(void *private_data),
+ void (*after_connect)(void *private_data),
+ void *private_data)
{
struct tevent_req *result;
struct async_connect_state *state;
@@ -258,6 +264,9 @@ struct tevent_req *async_connect_send(TALLOC_CTX *mem_ctx,
state->fd = fd;
state->sys_errno = 0;
+ state->before_connect = before_connect;
+ state->after_connect = after_connect;
+ state->private_data = private_data;
state->old_sockflags = fcntl(fd, F_GETFL, 0);
if (state->old_sockflags == -1) {
@@ -273,7 +282,16 @@ struct tevent_req *async_connect_send(TALLOC_CTX *mem_ctx,
set_blocking(fd, false);
+ if (state->before_connect != NULL) {
+ state->before_connect(state->private_data);
+ }
+
state->result = connect(fd, address, address_len);
+
+ if (state->after_connect != NULL) {
+ state->after_connect(state->private_data);
+ }
+
if (state->result == 0) {
tevent_req_done(result);
goto done;
@@ -328,8 +346,17 @@ static void async_connect_connected(struct tevent_context *ev,
tevent_req_data(req, struct async_connect_state);
int ret;
+ if (state->before_connect != NULL) {
+ state->before_connect(state->private_data);
+ }
+
ret = connect(state->fd, (struct sockaddr *)(void *)&state->address,
state->address_len);
+
+ if (state->after_connect != NULL) {
+ state->after_connect(state->private_data);
+ }
+
if (ret == 0) {
state->sys_errno = 0;
TALLOC_FREE(fde);
diff --git a/lib/async_req/async_sock.h b/lib/async_req/async_sock.h
index af917bc..494b92e 100644
--- a/lib/async_req/async_sock.h
+++ b/lib/async_req/async_sock.h
@@ -40,10 +40,12 @@ struct tevent_req *recvfrom_send(TALLOC_CTX *mem_ctx,
socklen_t *addr_len);
ssize_t recvfrom_recv(struct tevent_req *req, int *perrno);
-struct tevent_req *async_connect_send(TALLOC_CTX *mem_ctx,
- struct tevent_context *ev,
- int fd, const struct sockaddr *address,
- socklen_t address_len);
+struct tevent_req *async_connect_send(
+ TALLOC_CTX *mem_ctx, struct tevent_context *ev, int fd,
+ const struct sockaddr *address, socklen_t address_len,
+ void (*before_connect)(void *private_data),
+ void (*after_connect)(void *private_data),
+ void *private_data);
int async_connect_recv(struct tevent_req *req, int *perrno);
struct tevent_req *writev_send(TALLOC_CTX *mem_ctx, struct tevent_context *ev,
diff --git a/source3/lib/ctdb_conn.c b/source3/lib/ctdb_conn.c
index a96615f..d7bf6a5 100644
--- a/source3/lib/ctdb_conn.c
+++ b/source3/lib/ctdb_conn.c
@@ -83,7 +83,7 @@ struct tevent_req *ctdb_conn_init_send(TALLOC_CTX *mem_ctx,
subreq = async_connect_send(state, ev, state->conn->fd,
(struct sockaddr *)&state->addr,
- sizeof(state->addr));
+ sizeof(state->addr), NULL, NULL, NULL);
if (tevent_req_nomem(subreq, req)) {
return tevent_req_post(req, ev);
}
diff --git a/source3/lib/util_sock.c b/source3/lib/util_sock.c
index 8f212e5..eb38055 100644
--- a/source3/lib/util_sock.c
+++ b/source3/lib/util_sock.c
@@ -586,7 +586,7 @@ struct tevent_req *open_socket_out_send(TALLOC_CTX *mem_ctx,
subreq = async_connect_send(state, state->ev, state->fd,
(struct sockaddr *)&state->ss,
- state->salen);
+ state->salen, NULL, NULL, NULL);
if ((subreq == NULL)
|| !tevent_req_set_endtime(
subreq, state->ev,
@@ -638,7 +638,7 @@ static void open_socket_out_connected(struct tevent_req *subreq)
subreq = async_connect_send(state, state->ev, state->fd,
(struct sockaddr *)&state->ss,
- state->salen);
+ state->salen, NULL, NULL, NULL);
if (tevent_req_nomem(subreq, req)) {
return;
}
diff --git a/source3/libsmb/unexpected.c b/source3/libsmb/unexpected.c
index f537b3d..2c01bb7 100644
--- a/source3/libsmb/unexpected.c
+++ b/source3/libsmb/unexpected.c
@@ -514,7 +514,7 @@ struct tevent_req *nb_packet_reader_send(TALLOC_CTX *mem_ctx,
subreq = async_connect_send(state, ev, state->reader->sock,
(struct sockaddr *)(void *)&state->addr,
- sizeof(state->addr));
+ sizeof(state->addr), NULL, NULL, NULL);
if (tevent_req_nomem(subreq, req)) {
return tevent_req_post(req, ev);
}
diff --git a/source3/torture/wbc_async.c b/source3/torture/wbc_async.c
index 9252b58..71e4de7 100644
--- a/source3/torture/wbc_async.c
+++ b/source3/torture/wbc_async.c
@@ -288,7 +288,7 @@ static struct tevent_req *wb_connect_send(TALLOC_CTX *mem_ctx,
subreq = async_connect_send(mem_ctx, ev, wb_ctx->fd,
(struct sockaddr *)(void *)&sunaddr,
- sizeof(sunaddr));
+ sizeof(sunaddr), NULL, NULL, NULL);
if (subreq == NULL) {
goto nomem;
}
--
1.7.9.5
More information about the samba-technical
mailing list