netdev - act_mirred: remove spinlock in fast path

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-next>] [day] [month] [year] [list]

Message-ID: <CAM_iQpU42hydN_akrr8EGuN8OGifyjM3j+W44hjOvRxxM52HbA@mail.gmail.com>
Date:	Fri, 17 Jun 2016 14:03:41 -0700
From:	Cong Wang <xiyou.wangcong@...il.com>
To:	Eric Dumazet <edumazet@...gle.com>,
	Jamal Hadi Salim <jhs@...atatu.com>,
	David Miller <davem@...emloft.net>
Cc:	Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: act_mirred: remove spinlock in fast path

Hi, Eric

During code review, I notice we might have some problem after we go
lockless for the fast path in act_mirred.

That is, what prevents us from the following possible race condition?

change a standalone action with tcf_mirred_init():
  // search for an existing action in hash
  // found it and got struct tcf_common
  m = to_mirred(a);
  m->tcf_action = parm->action;
  // Interrupted by BH

tcf_mirred() jumps in:
  rcu_read_lock()
  retval = READ_ONCE(m->tcf_action);
  if (m->tcfm_eaction != TCA_EGRESS_MIRROR)
  ....
  rcu_unread_lock()

now go back to tcf_mirred_init():
  m->tcfm_eaction = parm->eaction;
  ....

IOW, the fast path could read a partially written change which could
be a problem? We need to allocate a new copy and then replace the old
one with it via RCU, don't we?

I can work on some patches, I want to make sure I don't miss anything here.

Thanks!