Skip to content

Commit

Permalink
Fix the race between tfw_sock_clnt_new() callback and
Browse files Browse the repository at this point in the history 
tfw_sock_clnt_start(): while a new child socket is created and
initialized by the listening socket, the listening socket, whcih was
removed from the reloaded configuration, is freed during the reconfiguration.
There actually was no requirement to use the listening socket in the
child initialization. The patch removes `listener` from SsProto as
doubling TfwListenSock->sk, so all connections will use smaller memory.

Also remove the unused synchronous sockets tests.
  • Loading branch information
@krizhanovsky
krizhanovsky committed Sep 4, 2022
1 parent 2f3a6a3 commit e8cfb48
Show file tree
Hide file tree
Showing 11 changed files with 66 additions and 1,450 deletions.
47 changes: 14 additions & 33 deletions fw/sock.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -969,9 +969,13 @@ ss_tcp_state_change(struct sock *sk)
TFW_VALIDATE_SK_LOCK_OWNER(sk);

if (sk->sk_state == TCP_ESTABLISHED) {
/* Process the new TCP connection. */
SsProto *proto = sk->sk_user_data;
struct sock *lsk = proto->listener;
/*
* Process the new TCP connection.
* The kernel sets sk_allocation to GFP_KERNEL, so this way we
* cad differentiate server sockets, created by as, and client
* sockets created by the kernel.
*/
bool is_srv_sock = (sk->sk_allocation == GFP_ATOMIC);
int r;

/*
Expand All @@ -993,12 +997,12 @@ ss_tcp_state_change(struct sock *sk)
* cannot be completed now. Paired with ss_connect()
* and ss_active_guard_enter() there.
*/
if (!lsk)
if (is_srv_sock)
ss_conn_drop_guard_exit(sk);
return;
}

if (lsk && ss_active_guard_enter(SS_V_ACT_LIVECONN)) {
if (!is_srv_sock && ss_active_guard_enter(SS_V_ACT_LIVECONN)) {
ss_do_close(sk, 0);
sock_put(sk);
ss_active_guard_exit(SS_V_ACT_NEWCONN);
Expand All @@ -1024,16 +1028,11 @@ ss_tcp_state_change(struct sock *sk)
}

sock_set_flag(sk, SOCK_TEMPESTA);
if (lsk) {
/*
* This is a new socket for an accepted connect
* request that the kernel has allocated itself.
* Kernel initializes this field to GFP_KERNEL.
* Tempesta works with sockets in SoftIRQ context,
* so set it to atomic allocation.
*/
sk->sk_allocation = GFP_ATOMIC;
}
/*
* Tempesta works with sockets in SoftIRQ context, so always use
* atomic allocations only.
*/
sk->sk_allocation = GFP_ATOMIC;
ss_active_guard_exit(SS_V_ACT_NEWCONN);
}
else if (sk->sk_state == TCP_CLOSE_WAIT) {
Expand Down Expand Up @@ -1079,20 +1078,6 @@ ss_tcp_state_change(struct sock *sk)
}
}

void
ss_proto_init(SsProto *proto, const SsHooks *hooks, int type)
{
proto->hooks = hooks;
proto->type = type;

/*
* The memory allocated for @proto should be already zero'ed, so don't
* initialize this field to NULL, but instead check the invariant.
*/
WARN_ON_ONCE(proto->listener);
}
EXPORT_SYMBOL(ss_proto_init);

/**
* Make data socket serviced by synchronous sockets.
*
Expand Down Expand Up @@ -1126,12 +1111,9 @@ EXPORT_SYMBOL(ss_set_callbacks);
void
ss_set_listen(struct sock *sk)
{
((SsProto *)sk->sk_user_data)->listener = sk;

sk->sk_state_change = ss_tcp_state_change;
sock_set_flag(sk, SOCK_TEMPESTA);
}
EXPORT_SYMBOL(ss_set_listen);

/*
* Create a new socket for IPv4 or IPv6 protocol. The original functions
Expand Down Expand Up @@ -1547,7 +1529,6 @@ ss_wait_newconn(void)
}
}
}
EXPORT_SYMBOL(ss_wait_newconn);

/**
* Wait until there are no queued works and no running tasklets.
Expand Down
81 changes: 49 additions & 32 deletions fw/sock_clnt.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,16 +157,17 @@ tfw_cli_conn_send(TfwCliConn *cli_conn, TfwMsg *msg)
return r;
}

static const SsHooks *tfw_sock_clnt_hooks(int type);

/**
* This hook is called when a new client connection is established.
*/
static int
tfw_sock_clnt_new(struct sock *sk)
{
int r = -ENOMEM;
int r = -ENOMEM, type;
TfwClient *cli;
TfwConn *conn;
SsProto *listen_sock_proto;
TfwAddr addr;

T_DBG3("new client socket: sk=%p, state=%u\n", sk, sk->sk_state);
Expand All @@ -178,7 +179,7 @@ tfw_sock_clnt_new(struct sock *sk)
* from referencing TfwListenSock{} while a new TfwConn{} object
* is not yet allocated/initialized.
*/
listen_sock_proto = sk->sk_user_data;
type = (long)sk->sk_user_data;
tfw_connection_unlink_from_sk(sk);

ss_getpeername(sk, &addr);
Expand All @@ -188,13 +189,13 @@ tfw_sock_clnt_new(struct sock *sk)
return -ENOENT;
}

conn = (TfwConn *)tfw_cli_conn_alloc(listen_sock_proto->type);
conn = (TfwConn *)tfw_cli_conn_alloc(type);
if (!conn) {
T_ERR("can't allocate a new client connection\n");
goto err_client;
}

ss_proto_inherit(listen_sock_proto, &conn->proto);
ss_proto_init(&conn->proto, tfw_sock_clnt_hooks(type), type);
BUG_ON(!(conn->proto.type & Conn_Clnt));

conn->destructor = (void *)tfw_cli_conn_release;
Expand Down Expand Up @@ -297,6 +298,24 @@ static const SsHooks tfw_sock_tls_clnt_ss_hooks = {
.connection_recv = tfw_tls_connection_recv,
};

static const SsHooks *
tfw_sock_clnt_hooks(int type)
{
switch (type) {
case TFW_FSM_HTTP:
return &tfw_sock_http_clnt_ss_hooks;
case TFW_FSM_HTTPS:
case TFW_FSM_H2:
/*
* We call the same TLS hooks before generic HTTP processing
* for both the HTTP/1 and HTTP/2.
*/
return &tfw_sock_tls_clnt_ss_hooks;
default:
BUG();
}
}

static int
__cli_conn_close_cb(TfwConn *conn)
{
Expand Down Expand Up @@ -383,23 +402,7 @@ static int
tfw_listen_sock_add(const TfwAddr *addr, int type)
{
TfwListenSock *ls;
const SsHooks *shooks;

switch (type) {
case TFW_FSM_HTTP:
shooks = &tfw_sock_http_clnt_ss_hooks;
break;
case TFW_FSM_HTTPS:
case TFW_FSM_H2:
/*
* We call the same TLS hooks before generic HTTP processing
* for both the HTTP/1 and HTTP/2.
*/
shooks = &tfw_sock_tls_clnt_ss_hooks;
break;
default:
return -EINVAL;
}
const SsHooks *shooks = tfw_sock_clnt_hooks(type);

/* Is there such an address on the list already? */
list_for_each_entry(ls, &tfw_listen_socks_reconf, list) {
Expand Down Expand Up @@ -471,15 +474,19 @@ tfw_listen_sock_start(TfwListenSock *ls)

/*
* Link the new socket and TfwListenSock.
* That must be done before calling ss_set_listen() that uses SsProto.
*
* sk_user_data for listening sockets is used as an inherited type for
* children sockets only, so we just store the socket type here.
* This way initialization of passively open sockets doesn't depend
* on the listening socket, which migh be closed during a new connection
* establishing.
*
* When a listening socket is closed, the children sockets migh live for
* an unlimited time.
*/
ls->sk = sk;
sk->sk_user_data = ls;
sk->sk_user_data = (void *)(long)ls->proto.type;

/*
* For listening sockets we use
* ss_set_listen() instead of ss_set_callbacks().
*/
ss_set_listen(sk);

inet_sk(sk)->freebind = 1;
Expand Down Expand Up @@ -718,9 +725,16 @@ tfw_sock_clnt_start(void)
listen_socks_sz--;

list_del(&ls->list);
if (!ls->sk)
continue;
ss_release(ls->sk);
if (ls->sk) {
ss_release(ls->sk);
/*
* There is at least one listener, which we need to close, so
* wait while all new connections finish before freeing the
* listeners. This prevents racing of the function with
* tfw_sock_clnt_new().
*/
ss_wait_newconn();
}
kfree(ls);
}

Expand Down Expand Up @@ -748,7 +762,10 @@ tfw_sock_clnt_stop(void)

might_sleep();

/* Stop listening sockets. */
/*
* Stop listening sockets, but leave them in the list to bve freed by
* tfw_cfgop_cleanup_sock_clnt().
*/
list_for_each_entry(ls, &tfw_listen_socks, list) {
if (!ls->sk)
continue;
Expand Down
10 changes: 3 additions & 7 deletions fw/sync_socket.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
/* Protocol descriptor. */
typedef struct ss_proto_t {
const struct ss_hooks *hooks;
struct sock *listener;
int type;
} SsProto;

Expand Down Expand Up @@ -106,9 +105,10 @@ ss_sock_live(struct sock *sk)
}

static inline void
ss_proto_inherit(const SsProto *parent, SsProto *child)
ss_proto_init(SsProto *proto, const SsHooks *hooks, int type)
{
*child = *parent;
proto->hooks = hooks;
proto->type = type;
}

/* Synchronous operation required. */
Expand All @@ -127,10 +127,6 @@ ss_proto_inherit(const SsProto *parent, SsProto *child)
#define SS_SKB_TYPE2F(t) (((int)(t)) << 8)
#define SS_SKB_F2TYPE(f) ((f) >> 8)

int ss_hooks_register(SsHooks* hooks);
void ss_hooks_unregister(SsHooks* hooks);

void ss_proto_init(SsProto *proto, const SsHooks *hooks, int type);
void ss_set_callbacks(struct sock *sk);
void ss_set_listen(struct sock *sk);
int ss_send(struct sock *sk, struct sk_buff **skb_head, int flags);
Expand Down
1 change: 0 additions & 1 deletion fw/t/bomber.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ static TfwBmbTask *bmb_tasks;
static inline void
__check_conn(TfwBmbConn *conn)
{
BUG_ON(conn->proto.listener);
BUG_ON(conn->proto.hooks != &bmb_hooks);
BUG_ON(!conn->sk);
BUG_ON(!conn->task);
Expand Down
59 changes: 0 additions & 59 deletions fw/t/sync_sockets/Makefile
View file

This file was deleted.

Loading

0 comments on commit e8cfb48

Please sign in to comment.