Skip to content

Commit

Permalink
afs: Refcount the afs_call struct
Browse files Browse the repository at this point in the history 
A static checker warning occurs in the AFS filesystem:

	fs/afs/cmservice.c:155 SRXAFSCB_CallBack()
	error: dereferencing freed memory 'call'

due to the reply being sent before we access the server it points to.  The
act of sending the reply causes the call to be freed if an error occurs
(but not if it doesn't).

On top of this, the lifetime handling of afs_call structs is fragile
because they get passed around through workqueues without any sort of
refcounting.

Deal with the issues by:

 (1) Fix the maybe/maybe not nature of the reply sending functions with
     regards to whether they release the call struct.

 (2) Refcount the afs_call struct and sort out places that need to get/put
     references.

 (3) Pass a ref through the work queue and release (or pass on) that ref in
     the work function.  Care has to be taken because a work queue may
     already own a ref to the call.

 (4) Do the cleaning up in the put function only.

 (5) Simplify module cleanup by always incrementing afs_outstanding_calls
     whenever a call is allocated.

 (6) Set the backlog to 0 with kernel_listen() at the beginning of the
     process of closing the socket to prevent new incoming calls from
     occurring and to remove the contribution of preallocated calls from
     afs_outstanding_calls before we wait on it.

A tracepoint is also added to monitor the afs_call refcount and lifetime.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Fixes: 08e0e7c: "[AF_RXRPC]: Make the in-kernel AFS filesystem use AF_RXRPC."
  • Loading branch information
David Howells committed Jan 9, 2017
1 parent 210f035 commit 341f741
Show file tree
Hide file tree
Showing 4 changed files with 199 additions and 79 deletions.
41 changes: 22 additions & 19 deletions fs/afs/cmservice.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ static int afs_deliver_cb_callback(struct afs_call *);
static int afs_deliver_cb_probe_uuid(struct afs_call *);
static int afs_deliver_cb_tell_me_about_yourself(struct afs_call *);
static void afs_cm_destructor(struct afs_call *);
static void SRXAFSCB_CallBack(struct work_struct *);
static void SRXAFSCB_InitCallBackState(struct work_struct *);
static void SRXAFSCB_Probe(struct work_struct *);
static void SRXAFSCB_ProbeUuid(struct work_struct *);
static void SRXAFSCB_TellMeAboutYourself(struct work_struct *);

#define CM_NAME(name) \
const char afs_SRXCB##name##_name[] __tracepoint_string = \
Expand All @@ -38,6 +43,7 @@ static const struct afs_call_type afs_SRXCBCallBack = {
.deliver = afs_deliver_cb_callback,
.abort_to_error = afs_abort_to_error,
.destructor = afs_cm_destructor,
.work = SRXAFSCB_CallBack,
};

/*
Expand All @@ -49,6 +55,7 @@ static const struct afs_call_type afs_SRXCBInitCallBackState = {
.deliver = afs_deliver_cb_init_call_back_state,
.abort_to_error = afs_abort_to_error,
.destructor = afs_cm_destructor,
.work = SRXAFSCB_InitCallBackState,
};

/*
Expand All @@ -60,6 +67,7 @@ static const struct afs_call_type afs_SRXCBInitCallBackState3 = {
.deliver = afs_deliver_cb_init_call_back_state3,
.abort_to_error = afs_abort_to_error,
.destructor = afs_cm_destructor,
.work = SRXAFSCB_InitCallBackState,
};

/*
Expand All @@ -71,6 +79,7 @@ static const struct afs_call_type afs_SRXCBProbe = {
.deliver = afs_deliver_cb_probe,
.abort_to_error = afs_abort_to_error,
.destructor = afs_cm_destructor,
.work = SRXAFSCB_Probe,
};

/*
Expand All @@ -82,6 +91,7 @@ static const struct afs_call_type afs_SRXCBProbeUuid = {
.deliver = afs_deliver_cb_probe_uuid,
.abort_to_error = afs_abort_to_error,
.destructor = afs_cm_destructor,
.work = SRXAFSCB_ProbeUuid,
};

/*
Expand All @@ -93,6 +103,7 @@ static const struct afs_call_type afs_SRXCBTellMeAboutYourself = {
.deliver = afs_deliver_cb_tell_me_about_yourself,
.abort_to_error = afs_abort_to_error,
.destructor = afs_cm_destructor,
.work = SRXAFSCB_TellMeAboutYourself,
};

/*
Expand Down Expand Up @@ -163,6 +174,7 @@ static void SRXAFSCB_CallBack(struct work_struct *work)
afs_send_empty_reply(call);

afs_break_callbacks(call->server, call->count, call->request);
afs_put_call(call);
_leave("");
}

Expand Down Expand Up @@ -284,9 +296,7 @@ static int afs_deliver_cb_callback(struct afs_call *call)
return -ENOTCONN;
call->server = server;

INIT_WORK(&call->work, SRXAFSCB_CallBack);
queue_work(afs_wq, &call->work);
return 0;
return afs_queue_call_work(call);
}

/*
Expand All @@ -300,6 +310,7 @@ static void SRXAFSCB_InitCallBackState(struct work_struct *work)

afs_init_callback_state(call->server);
afs_send_empty_reply(call);
afs_put_call(call);
_leave("");
}

Expand Down Expand Up @@ -330,9 +341,7 @@ static int afs_deliver_cb_init_call_back_state(struct afs_call *call)
return -ENOTCONN;
call->server = server;

INIT_WORK(&call->work, SRXAFSCB_InitCallBackState);
queue_work(afs_wq, &call->work);
return 0;
return afs_queue_call_work(call);
}

/*
Expand Down Expand Up @@ -404,9 +413,7 @@ static int afs_deliver_cb_init_call_back_state3(struct afs_call *call)
return -ENOTCONN;
call->server = server;

INIT_WORK(&call->work, SRXAFSCB_InitCallBackState);
queue_work(afs_wq, &call->work);
return 0;
return afs_queue_call_work(call);
}

/*
Expand All @@ -418,6 +425,7 @@ static void SRXAFSCB_Probe(struct work_struct *work)

_enter("");
afs_send_empty_reply(call);
afs_put_call(call);
_leave("");
}

Expand All @@ -437,9 +445,7 @@ static int afs_deliver_cb_probe(struct afs_call *call)
/* no unmarshalling required */
call->state = AFS_CALL_REPLYING;

INIT_WORK(&call->work, SRXAFSCB_Probe);
queue_work(afs_wq, &call->work);
return 0;
return afs_queue_call_work(call);
}

/*
Expand All @@ -462,6 +468,7 @@ static void SRXAFSCB_ProbeUuid(struct work_struct *work)
reply.match = htonl(1);

afs_send_simple_reply(call, &reply, sizeof(reply));
afs_put_call(call);
_leave("");
}

Expand Down Expand Up @@ -520,9 +527,7 @@ static int afs_deliver_cb_probe_uuid(struct afs_call *call)

call->state = AFS_CALL_REPLYING;

INIT_WORK(&call->work, SRXAFSCB_ProbeUuid);
queue_work(afs_wq, &call->work);
return 0;
return afs_queue_call_work(call);
}

/*
Expand Down Expand Up @@ -584,7 +589,7 @@ static void SRXAFSCB_TellMeAboutYourself(struct work_struct *work)
reply.cap.capcount = htonl(1);
reply.cap.caps[0] = htonl(AFS_CAP_ERROR_TRANSLATION);
afs_send_simple_reply(call, &reply, sizeof(reply));

afs_put_call(call);
_leave("");
}

Expand All @@ -604,7 +609,5 @@ static int afs_deliver_cb_tell_me_about_yourself(struct afs_call *call)
/* no unmarshalling required */
call->state = AFS_CALL_REPLYING;

INIT_WORK(&call->work, SRXAFSCB_TellMeAboutYourself);
queue_work(afs_wq, &call->work);
return 0;
return afs_queue_call_work(call);
}
9 changes: 8 additions & 1 deletion fs/afs/internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ enum afs_call_state {
struct afs_call {
const struct afs_call_type *type; /* type of call */
wait_queue_head_t waitq; /* processes awaiting completion */
struct work_struct async_work; /* asynchronous work processor */
struct work_struct async_work; /* async I/O processor */
struct work_struct work; /* actual work processor */
struct rxrpc_call *rxcall; /* RxRPC call handle */
struct key *key; /* security for this call */
Expand All @@ -82,6 +82,7 @@ struct afs_call {
pgoff_t first; /* first page in mapping to deal with */
pgoff_t last; /* last page in mapping to deal with */
size_t offset; /* offset into received data store */
atomic_t usage;
enum afs_call_state state;
int error; /* error code */
u32 abort_code; /* Remote abort ID or 0 */
Expand Down Expand Up @@ -115,6 +116,9 @@ struct afs_call_type {

/* clean up a call */
void (*destructor)(struct afs_call *call);

/* Work function */
void (*work)(struct work_struct *work);
};

/*
Expand Down Expand Up @@ -591,9 +595,12 @@ extern void afs_proc_cell_remove(struct afs_cell *);
* rxrpc.c
*/
extern struct socket *afs_socket;
extern atomic_t afs_outstanding_calls;

extern int afs_open_socket(void);
extern void afs_close_socket(void);
extern void afs_put_call(struct afs_call *);
extern int afs_queue_call_work(struct afs_call *);
extern int afs_make_call(struct in_addr *, struct afs_call *, gfp_t, bool);
extern struct afs_call *afs_alloc_flat_call(const struct afs_call_type *,
size_t, size_t);
Expand Down
Loading

0 comments on commit 341f741

Please sign in to comment.