linux-kernel - Re: [PATCH 2/2] staging: greybus: loopback: convert loopback to use generic async operations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 5 Nov 2017 13:24:04 +0100
From:   Johan Hovold <johan@...nel.org>
To:     Bryan O'Donoghue <pure.logic@...us-software.ie>
Cc:     johan@...nel.org, elder@...nel.org, gregkh@...uxfoundation.org,
        devel@...verdev.osuosl.org, keescook@...omium.org,
        linux-kernel@...r.kernel.org, greybus-dev@...ts.linaro.org
Subject: Re: [PATCH 2/2] staging: greybus: loopback: convert loopback to use
 generic async operations

On Sun, Nov 05, 2017 at 03:27:39AM +0000, Bryan O'Donoghue wrote:
> Loopback has its own internal method for tracking and timing out
> asynchronous operations however previous patches make it possible to use
> functionality provided by operation.c to do this instead. Using the code in
> operation.c means we can completely subtract the timer, the work-queue, the
> kref and the cringe-worthy 'pending' flag. The completion callback
> triggered by operation.c will provide an authoritative result code -
> including -ETIMEDOUT for asynchronous operations.

Thanks for respinning this one, Bryan. Nice diff stat too.

Note however that this patch depends on Arnd's

	[PATCH 2/2] staging: greybus/loopback: use ktime_get() for time intervals

which hasn't been applied yet.

> Signed-off-by: Bryan O'Donoghue <pure.logic@...us-software.ie>
> Cc: Johan Hovold <johan@...nel.org>
> Cc: Alex Elder <elder@...nel.org>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: greybus-dev@...ts.linaro.org
> Cc: devel@...verdev.osuosl.org
> Cc: linux-kernel@...r.kernel.org
> ---

I double checked against v3 and it seems nothing has changed except for
you rebasing it against the latest staging-next (+ Arnd's patch).

A changelog entry mentioning that here would be nice.

So this all still looks good to me except for the two comments I had on
v3 (repeated below).

>  drivers/staging/greybus/loopback.c | 165 +++++++------------------------------
>  1 file changed, 31 insertions(+), 134 deletions(-)

>  static int gb_loopback_async_operation(struct gb_loopback *gb, int type,
> @@ -575,15 +478,11 @@ static int gb_loopback_async_operation(struct gb_loopback *gb, int type,
>  	struct gb_loopback_async_operation *op_async;
>  	struct gb_operation *operation;
>  	int ret;
> -	unsigned long flags;
>  
>  	op_async = kzalloc(sizeof(*op_async), GFP_KERNEL);
>  	if (!op_async)
>  		return -ENOMEM;
>  
> -	INIT_WORK(&op_async->work, gb_loopback_async_operation_work);
> -	kref_init(&op_async->kref);
> -
>  	operation = gb_operation_create(gb->connection, type, request_size,
>  					response_size, GFP_KERNEL);
>  	if (!operation) {
> @@ -594,33 +493,29 @@ static int gb_loopback_async_operation(struct gb_loopback *gb, int type,
>  	if (request_size)
>  		memcpy(operation->request->payload, request, request_size);
>  
> +	gb_operation_set_data(operation, op_async);
> +
>  	op_async->gb = gb;
>  	op_async->operation = operation;
>  	op_async->completion = completion;
>  
> -	spin_lock_irqsave(&gb_dev.lock, flags);
> -	list_add_tail(&op_async->entry, &gb_dev.list_op_async);
> -	spin_unlock_irqrestore(&gb_dev.lock, flags);
> -
>  	op_async->ts = ktime_get();
> -	op_async->pending = true;
> +
>  	atomic_inc(&gb->outstanding_operations);
> +
>  	mutex_lock(&gb->mutex);

This lock does not seem to be needed here any more.

>  	ret = gb_operation_request_send(operation,
>  					gb_loopback_async_operation_callback,
> -					0,
> +					jiffies_to_msecs(gb->jiffy_timeout),
>  					GFP_KERNEL);
>  	if (ret)
>  		goto error;
>  
> -	setup_timer(&op_async->timer, gb_loopback_async_operation_timeout,
> -			(unsigned long)operation->id);
> -	op_async->timer.expires = jiffies + gb->jiffy_timeout;
> -	add_timer(&op_async->timer);
> -
>  	goto done;
>  error:
> -	gb_loopback_async_operation_put(op_async);
> +	atomic_dec(&gb->outstanding_operations);
> +	gb_operation_put(operation);
> +	kfree(op_async);
>  done:
>  	mutex_unlock(&gb->mutex);
>  	return ret;
> @@ -1024,8 +919,10 @@ static int gb_loopback_fn(void *data)
>  			else if (type == GB_LOOPBACK_TYPE_SINK)
>  				error = gb_loopback_async_sink(gb, size);
>  
> -			if (error)
> +			if (error) {
>  				gb->error++;
> +				gb->iteration_count++;
> +			}

And this looks like an unrelated bug fix that should go in it's own
patch, right?

The iteration count should be incremented on errors regardless of this
change.

Also you probably want to hold the gb->mutex while doing so (also in the
sync case).

>  		} else {
>  			/* We are effectively single threaded here */
>  			if (type == GB_LOOPBACK_TYPE_PING)

Thanks,
Johan