[PATCH linux dev-4.10 v2 7/9] drivers: fsi: occ: Fix client memory management
Andrew Jeffery
andrew at aj.id.au
Thu Oct 5 12:12:41 AEDT 2017
On Fri, 2017-09-29 at 17:41 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames at us.ibm.com>
>
> Potential for bad memory access in the worker function. Now fixed by
> using reference counters.
>
> Signed-off-by: Edward A. James <eajames at us.ibm.com>
> ---
> drivers/fsi/occ.c | 92 ++++++++++++++++++++++++++-----------------------------
> 1 file changed, 43 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
> index 550ae11..c3d9976 100644
> --- a/drivers/fsi/occ.c
> +++ b/drivers/fsi/occ.c
> @@ -70,15 +70,11 @@ struct occ_response {
> * and cleared if the transfer fails or occ_worker_getsram completes.
> * XFR_COMPLETE is set when a transfer fails or finishes occ_worker_getsram.
> * XFR_CANCELED is set when the transfer's client is released.
> - * XFR_WAITING is set from read() if the transfer isn't complete and
> - * O_NONBLOCK wasn't specified. Cleared in read() when transfer completes or
> - * fails.
> */
> enum {
> XFR_IN_PROGRESS,
> XFR_COMPLETE,
> XFR_CANCELED,
> - XFR_WAITING,
> };
>
> struct occ_xfr {
> @@ -104,6 +100,7 @@ enum {
> };
>
> struct occ_client {
> + struct kref kref;
> struct occ *occ;
> struct occ_xfr xfr;
> spinlock_t lock; /* lock access to the client state */
> @@ -140,6 +137,24 @@ static int occ_enqueue_xfr(struct occ_xfr *xfr)
> return 0;
> }
>
> +static void occ_get_client(struct occ_client *client)
> +{
> + kref_get(&client->kref);
> +}
> +
> +static void occ_client_release(struct kref *kref)
> +{
> + struct occ_client *client = container_of(kref, struct occ_client,
> + kref);
> +
> + kfree(client);
> +}
> +
> +static void occ_put_client(struct occ_client *client)
> +{
> + kref_put(&client->kref, occ_client_release);
> +}
> +
> static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
> {
> struct occ_client *client;
> @@ -152,6 +167,7 @@ static struct occ_client *occ_open_common(struct occ *occ, unsigned long flags)
> return ERR_PTR(-ENOMEM);
>
> client->occ = occ;
> + kref_init(&client->kref);
> spin_lock_init(&client->lock);
> init_waitqueue_head(&client->wait);
>
> @@ -187,6 +203,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
> if (len > OCC_SRAM_BYTES)
> return -EINVAL;
>
> + occ_get_client(client);
> spin_lock_irq(&client->lock);
>
> if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {
> @@ -207,8 +224,6 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
> goto done;
> }
>
> - set_bit(XFR_WAITING, &xfr->flags);
> -
> spin_unlock_irq(&client->lock);
>
> rc = wait_event_interruptible(client->wait,
> @@ -220,15 +235,12 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>
> spin_lock_irq(&client->lock);
>
> - if (test_bit(XFR_CANCELED, &xfr->flags)) {
> - spin_unlock_irq(&client->lock);
> - kfree(client);
> - return -EBADFD;
> - }
> -
> - clear_bit(XFR_WAITING, &xfr->flags);
> if (!test_bit(XFR_COMPLETE, &xfr->flags)) {
> - rc = -EINTR;
> + if (occ->cancel || test_bit(XFR_CANCELED, &xfr->flags))
> + rc = -ECANCELED;
> + else
> + rc = -EINTR;
> +
> goto done;
> }
> }
> @@ -259,6 +271,7 @@ static ssize_t occ_read_common(struct occ_client *client, char __user *ubuf,
>
> done:
> spin_unlock_irq(&client->lock);
> + occ_put_client(client);
> return rc;
> }
>
> @@ -282,6 +295,7 @@ static ssize_t occ_write_common(struct occ_client *client,
> if (len > (OCC_CMD_DATA_BYTES + 3) || len < 3)
> return -EINVAL;
>
> + occ_get_client(client);
> spin_lock_irq(&client->lock);
>
> if (test_bit(CLIENT_XFR_PENDING, &client->flags)) {
> @@ -330,6 +344,7 @@ static ssize_t occ_write_common(struct occ_client *client,
>
> done:
> spin_unlock_irq(&client->lock);
> + occ_put_client(client);
> return rc;
> }
>
> @@ -348,38 +363,26 @@ static int occ_release_common(struct occ_client *client)
>
> spin_lock_irq(&client->lock);
>
> - if (!test_bit(CLIENT_XFR_PENDING, &client->flags)) {
> - spin_unlock_irq(&client->lock);
> - kfree(client);
> - return 0;
> - }
> + set_bit(XFR_CANCELED, &xfr->flags);
> + if (!test_bit(CLIENT_XFR_PENDING, &client->flags))
> + goto done;
>
> spin_lock_irq(&occ->list_lock);
>
> - set_bit(XFR_CANCELED, &xfr->flags);
> if (!test_bit(XFR_IN_PROGRESS, &xfr->flags)) {
> /* already deleted from list if complete */
> if (!test_bit(XFR_COMPLETE, &xfr->flags))
> list_del(&xfr->link);
> -
> - spin_unlock_irq(&occ->list_lock);
> -
> - if (test_bit(XFR_WAITING, &xfr->flags)) {
> - /* blocking read; let reader clean up */
> - wake_up_interruptible(&client->wait);
> - spin_unlock_irq(&client->lock);
> - return 0;
> - }
> -
> - spin_unlock_irq(&client->lock);
> -
> - kfree(client);
> - return 0;
> }
>
> - /* operation is in progress; let worker clean up */
> spin_unlock_irq(&occ->list_lock);
> +
> + wake_up_interruptible(&client->wait);
I think we we want to be doing wake_up_all() here too?
Andrew
> +
> +done:
> spin_unlock_irq(&client->lock);
> +
> + occ_put_client(client);
> return 0;
> }
>
> @@ -582,7 +585,7 @@ static int occ_trigger_attn(struct device *sbefifo)
>
> static void occ_worker(struct work_struct *work)
> {
> - int rc = 0, empty, waiting, canceled;
> + int rc = 0, empty;
> u16 resp_data_length;
> unsigned long start;
> const unsigned long timeout = msecs_to_jiffies(OCC_TIMEOUT_MS);
> @@ -605,6 +608,8 @@ static void occ_worker(struct work_struct *work)
> return;
> }
>
> + client = to_client(xfr);
> + occ_get_client(client);
> resp = (struct occ_response *)xfr->buf;
> set_bit(XFR_IN_PROGRESS, &xfr->flags);
>
> @@ -660,29 +665,18 @@ static void occ_worker(struct work_struct *work)
> mutex_unlock(&occ->occ_lock);
>
> xfr->rc = rc;
> - client = to_client(xfr);
> -
> - /* lock client to prevent race with read() */
> - spin_lock_irq(&client->lock);
> -
> set_bit(XFR_COMPLETE, &xfr->flags);
> - waiting = test_bit(XFR_WAITING, &xfr->flags);
> -
> - spin_unlock_irq(&client->lock);
>
> spin_lock_irq(&occ->list_lock);
>
> clear_bit(XFR_IN_PROGRESS, &xfr->flags);
> list_del(&xfr->link);
> empty = list_empty(&occ->xfrs);
> - canceled = test_bit(XFR_CANCELED, &xfr->flags);
>
> spin_unlock_irq(&occ->list_lock);
>
> - if (waiting)
> - wake_up_interruptible(&client->wait);
> - else if (canceled)
> - kfree(client);
> + wake_up_interruptible(&client->wait);
> + occ_put_client(client);
>
> if (!empty)
> goto again;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20171005/11c0e7c1/attachment-0001.sig>
More information about the openbmc
mailing list