Linux Virtualization - Jan 2014 - [PATCH net-next v3 5/5] virtio-net: initial rx sysfs support, export mergeable rx buffer size

Sorry, just realized - I think disabling NAPI is necessary but not
sufficient. There is also the issue that refill_work() could be
scheduled. If refill_work() executes, it will re-enable NAPI. We'd need
to cancel the vi->refill delayed work to prevent this AFAICT, and also
ensure that no other function re-schedules vi->refill or re-enables NAPI
(virtnet_open/close, virtnet_set_queues, and virtnet_freeze/restore).

How is the following sequence of operations:
rtnl_lock();
cancel_delayed_work_sync(&vi->refill);
napi_disable(&rq->napi);
read rq->mrg_avg_pkt_len
virtnet_enable_napi();
rtnl_unlock();

Additionally, if we disable NAPI when reading this file, perhaps
the permissions should be changed to 400 so that an unprivileged
user cannot temporarily disable network RX processing by reading these
sysfs files. Does that sound reasonable?

Best,

Mike

On Thu, 2014-01-16 at 09:27 -0800, Michael Dalton wrote:
> Sorry, just realized - I think disabling NAPI is necessary but not
> sufficient. There is also the issue that refill_work() could be
> scheduled. If refill_work() executes, it will re-enable NAPI. We'd need
> to cancel the vi->refill delayed work to prevent this AFAICT, and also
> ensure that no other function re-schedules vi->refill or re-enables NAPI
> (virtnet_open/close, virtnet_set_queues, and virtnet_freeze/restore).
> 
> How is the following sequence of operations:
> rtnl_lock();
> cancel_delayed_work_sync(&vi->refill);
> napi_disable(&rq->napi);
> read rq->mrg_avg_pkt_len
> virtnet_enable_napi();
> rtnl_unlock();
> 
> Additionally, if we disable NAPI when reading this file, perhaps
> the permissions should be changed to 400 so that an unprivileged
> user cannot temporarily disable network RX processing by reading these
> sysfs files. Does that sound reasonable?
I think all this complexity makes no sense to me.

Who cares of sysfs reading a value that might be updated ?
This is purely a debugging utility.

As soon as you read the value, it might already have changed anyway.

Its a integer, just read it without special care.

atomic_read() has also same 'problem', and we do not care.

Make sure that a recompute (aka ewma_add()) does not store intermediate
wrong values, by using ACCESS_ONCE(), and thats enough. No need for the
seqcount overhead.

diff --git a/lib/average.c b/lib/average.c
index 99a67e662b3c..044e0b7f28a8 100644
--- a/lib/average.c
+++ b/lib/average.c
@@ -53,8 +53,10 @@ EXPORT_SYMBOL(ewma_init);
  */
 struct ewma *ewma_add(struct ewma *avg, unsigned long val)
 {
-	avg->internal = avg->internal  ?
-		(((avg->internal << avg->weight) - avg->internal) +
+	unsigned long internal = ACCESS_ONCE(avg->internal);
+
+	ACCESS_ONCE(avg->internal) = internal  ?
+		(((internal << avg->weight) - internal) +
 			(val << avg->factor)) >> avg->weight :
 		(val << avg->factor);
 	return avg;

On Thu, Jan 16, 2014 at 10:04:41AM -0800, Eric Dumazet
wrote:
> On Thu, 2014-01-16 at 09:27 -0800, Michael Dalton wrote:
> > Sorry, just realized - I think disabling NAPI is necessary but not
> > sufficient. There is also the issue that refill_work() could be
> > scheduled. If refill_work() executes, it will re-enable NAPI. We'd
need
> > to cancel the vi->refill delayed work to prevent this AFAICT, and
also
> > ensure that no other function re-schedules vi->refill or re-enables
NAPI
> > (virtnet_open/close, virtnet_set_queues, and virtnet_freeze/restore).
> > 
> > How is the following sequence of operations:
> > rtnl_lock();
> > cancel_delayed_work_sync(&vi->refill);
> > napi_disable(&rq->napi);
> > read rq->mrg_avg_pkt_len
> > virtnet_enable_napi();
> > rtnl_unlock();
> > 
> > Additionally, if we disable NAPI when reading this file, perhaps
> > the permissions should be changed to 400 so that an unprivileged
> > user cannot temporarily disable network RX processing by reading these
> > sysfs files. Does that sound reasonable?
> 
> I think all this complexity makes no sense to me.
> 
> Who cares of sysfs reading a value that might be updated ?
> This is purely a debugging utility.
> 
> As soon as you read the value, it might already have changed anyway.
> 
> Its a integer, just read it without special care.
That's fine too as far as I'm concerned.
> 
> atomic_read() has also same 'problem', and we do not care.
> 
> Make sure that a recompute (aka ewma_add()) does not store intermediate
> wrong values, by using ACCESS_ONCE(), and thats enough. No need for the
> seqcount overhead.
> 
> diff --git a/lib/average.c b/lib/average.c
> index 99a67e662b3c..044e0b7f28a8 100644
> --- a/lib/average.c
> +++ b/lib/average.c
> @@ -53,8 +53,10 @@ EXPORT_SYMBOL(ewma_init);
>   */
>  struct ewma *ewma_add(struct ewma *avg, unsigned long val)
>  {
> -	avg->internal = avg->internal  ?
> -		(((avg->internal << avg->weight) - avg->internal) +
> +	unsigned long internal = ACCESS_ONCE(avg->internal);
> +
> +	ACCESS_ONCE(avg->internal) = internal  ?
> +		(((internal << avg->weight) - internal) +
>  			(val << avg->factor)) >> avg->weight :
>  		(val << avg->factor);
>  	return avg;
>