From mboxrd@z Thu Jan 1 00:00:00 1970 From: humbabek Subject: [PATCH] move smp_rmb() after load of status Date: Sun, 8 Jul 2018 11:27:15 +0200 Message-ID: <20180708092718.17605-1-humbabek@gmail.com> References: <5e5dfd01-bad6-55b0-cfd1-75f7ec769e89@gmail.com> Cc: humbabek , "David S. Miller" , Willem de Bruijn , Eric Dumazet , Kirill Tkhai , Kees Cook , Mike Maloney , Magnus Karlsson , netdev@vger.kernel.org, linux-kernel@vger.kernel.org To: unlisted-recipients:; (no To-header on input) Return-path: In-Reply-To: <5e5dfd01-bad6-55b0-cfd1-75f7ec769e89@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org Signed-off-by: humbabek --- 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?