Skip to content

Commit

Permalink
AFS: Fix cache manager service handlers
Browse files Browse the repository at this point in the history
Fix the cache manager RPC service handlers.  The afs_send_empty_reply() and
afs_send_simple_reply() functions:

 (a) Kill the call and free up the buffers associated with it if they fail.

 (b) Return with call intact if it they succeed.

However, none of the callers actually check the result or clean up if
successful - and may use the now non-existent data if it fails.

This was detected by Dan Carpenter using a static checker:

	The patch 08e0e7c: "[AF_RXRPC]: Make the in-kernel AFS
	filesystem use AF_RXRPC." from Apr 26, 2007, leads to the following
	static checker warning:
	"fs/afs/cmservice.c:155 SRXAFSCB_CallBack()
		 warn: 'call' was already freed."

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: David Howells <dhowells@redhat.com>
  • Loading branch information
David Howells committed May 21, 2014
1 parent 60b5f90 commit 6c67c7c
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 22 deletions.
19 changes: 19 additions & 0 deletions fs/afs/cmservice.c
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,15 @@ static void afs_cm_destructor(struct afs_call *call)
{
_enter("");

/* Break the callbacks here so that we do it after the final ACK is
* received. The step number here must match the final number in
* afs_deliver_cb_callback().
*/
if (call->unmarshall == 6) {
ASSERT(call->server && call->count && call->request);
afs_break_callbacks(call->server, call->count, call->request);
}

afs_put_server(call->server);
call->server = NULL;
kfree(call->buffer);
Expand Down Expand Up @@ -272,6 +281,16 @@ static int afs_deliver_cb_callback(struct afs_call *call, struct sk_buff *skb,
_debug("trailer");
if (skb->len != 0)
return -EBADMSG;

/* Record that the message was unmarshalled successfully so
* that the call destructor can know do the callback breaking
* work, even if the final ACK isn't received.
*
* If the step number changes, then afs_cm_destructor() must be
* updated also.
*/
call->unmarshall++;
case 6:
break;
}

Expand Down
43 changes: 21 additions & 22 deletions fs/afs/rxrpc.c
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,19 @@ static void afs_free_call(struct afs_call *call)
kfree(call);
}

/*
* End a call
*/
static void afs_end_call(struct afs_call *call)
{
if (call->rxcall) {
rxrpc_kernel_end_call(call->rxcall);
call->rxcall = NULL;
}
call->type->destructor(call);
afs_free_call(call);
}

/*
* allocate a call with flat request and reply buffers
*/
Expand Down Expand Up @@ -383,11 +396,8 @@ int afs_make_call(struct in_addr *addr, struct afs_call *call, gfp_t gfp,
rxrpc_kernel_abort_call(rxcall, RX_USER_ABORT);
while ((skb = skb_dequeue(&call->rx_queue)))
afs_free_skb(skb);
rxrpc_kernel_end_call(rxcall);
call->rxcall = NULL;
error_kill_call:
call->type->destructor(call);
afs_free_call(call);
afs_end_call(call);
_leave(" = %d", ret);
return ret;
}
Expand Down Expand Up @@ -509,12 +519,8 @@ static void afs_deliver_to_call(struct afs_call *call)
if (call->state >= AFS_CALL_COMPLETE) {
while ((skb = skb_dequeue(&call->rx_queue)))
afs_free_skb(skb);
if (call->incoming) {
rxrpc_kernel_end_call(call->rxcall);
call->rxcall = NULL;
call->type->destructor(call);
afs_free_call(call);
}
if (call->incoming)
afs_end_call(call);
}

_leave("");
Expand Down Expand Up @@ -564,10 +570,7 @@ static int afs_wait_for_call_to_complete(struct afs_call *call)
}

_debug("call complete");
rxrpc_kernel_end_call(call->rxcall);
call->rxcall = NULL;
call->type->destructor(call);
afs_free_call(call);
afs_end_call(call);
_leave(" = %d", ret);
return ret;
}
Expand Down Expand Up @@ -790,10 +793,7 @@ void afs_send_empty_reply(struct afs_call *call)
_debug("oom");
rxrpc_kernel_abort_call(call->rxcall, RX_USER_ABORT);
default:
rxrpc_kernel_end_call(call->rxcall);
call->rxcall = NULL;
call->type->destructor(call);
afs_free_call(call);
afs_end_call(call);
_leave(" [error]");
return;
}
Expand Down Expand Up @@ -823,17 +823,16 @@ void afs_send_simple_reply(struct afs_call *call, const void *buf, size_t len)
call->state = AFS_CALL_AWAIT_ACK;
n = rxrpc_kernel_send_data(call->rxcall, &msg, len);
if (n >= 0) {
/* Success */
_leave(" [replied]");
return;
}

if (n == -ENOMEM) {
_debug("oom");
rxrpc_kernel_abort_call(call->rxcall, RX_USER_ABORT);
}
rxrpc_kernel_end_call(call->rxcall);
call->rxcall = NULL;
call->type->destructor(call);
afs_free_call(call);
afs_end_call(call);
_leave(" [error]");
}

Expand Down

0 comments on commit 6c67c7c

Please sign in to comment.