netdev - [PATCH] move smp_rmb() after load of status

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,  8 Jul 2018 11:27:15 +0200
From:   humbabek <humbabek@...il.com>
To:     unlisted-recipients:; (no To-header on input)
Cc:     humbabek <humbabek@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Willem de Bruijn <willemb@...gle.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Kirill Tkhai <ktkhai@...tuozzo.com>,
        Kees Cook <keescook@...omium.org>,
        Mike Maloney <maloney@...gle.com>,
        Magnus Karlsson <magnus.karlsson@...el.com>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [PATCH] move smp_rmb() after load of status

Signed-off-by: humbabek <humbabek@...il.com>
---
 net/packet/af_packet.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 57634bc3da74..91830ef07c48 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -394,25 +394,30 @@ static void __packet_set_status(struct packet_sock *po, void *frame, int status)
 static int __packet_get_status(struct packet_sock *po, void *frame)
 {
 	union tpacket_uhdr h;
-
-	smp_rmb();
+	int status;
 
 	h.raw = frame;
 	switch (po->tp_version) {
 	case TPACKET_V1:
 		flush_dcache_page(pgv_to_page(&h.h1->tp_status));
-		return h.h1->tp_status;
+		status = h.h1->tp_status;
+		break;
 	case TPACKET_V2:
 		flush_dcache_page(pgv_to_page(&h.h2->tp_status));
-		return h.h2->tp_status;
+		status = h.h2->tp_status;
+		break;
 	case TPACKET_V3:
 		flush_dcache_page(pgv_to_page(&h.h3->tp_status));
-		return h.h3->tp_status;
+		status = h.h3->tp_status;
+		break;
 	default:
 		WARN(1, "TPACKET version not supported.\n");
 		BUG();
-		return 0;
+		status = 0;
+		break;
 	}
+	smp_rmb();
+	return status;
 }
 
 static __u32 tpacket_get_timestamp(struct sk_buff *skb, struct timespec *ts,
-- 
2.17.1

Sorry for a lack of information- it is the first my patch. 

The opposie smp_wmb() is in: https://elixir.bootlin.com/linux/v4.15/source/net/packet/af_packet.c#L415

On my eye smp_rmb() should be moved *after* actual read of status to be ensured that no read after __packet_get_status() happens before actual read of status.

Note, that tpacket_snd() polls a ring [https://elixir.bootlin.com/linux/v4.15/source/net/packet/af_packet.c#L2677], possibly waiting for a published packet (by published I mean a packet with a TP_STATUS_SEND_REQUEST status).

It is important to have a guarantee that no load was reordered before we know the status, isn't it?

Please note that it is possible that I am wrong, so please take this into account. However, if I am wrong please let me know. 

>From the other side, what is the point to place smp_rmb() before read of status?