netdev - Re: [PATCH net-next v3 5/5] virtio-net: initial rx sysfs support, export mergeable rx buffer size

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:	Thu, 16 Jan 2014 10:04:41 -0800
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Michael Dalton <mwdalton@...gle.com>
Cc:	"Michael S. Tsirkin" <mst@...hat.com>,
	"David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
	Eric Dumazet <edumazet@...gle.com>,
	Rusty Russell <rusty@...tcorp.com.au>,
	Jason Wang <jasowang@...hat.com>,
	lf-virt <virtualization@...ts.linux-foundation.org>
Subject: Re: [PATCH net-next v3 5/5] virtio-net: initial rx sysfs support,
 export mergeable rx buffer size

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;


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html