netdev - Re: [PATCH V3] neigh: Really delete an arp/neigh entry on "ip neigh delete" or "arp -d"

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]

Message-ID: <alpine.LFD.2.20.1705312337280.5775@ja.home.ssi.bg>
Date:   Thu, 1 Jun 2017 00:41:33 +0300 (EEST)
From:   Julian Anastasov <ja@....bg>
To:     Sowmini Varadhan <sowmini.varadhan@...cle.com>
cc:     netdev@...r.kernel.org, davem@...emloft.net,
        stephen@...workplumber.org
Subject: Re: [PATCH V3] neigh: Really delete an arp/neigh entry on "ip neigh
 delete" or "arp -d"


	Hello,

On Tue, 30 May 2017, Sowmini Varadhan wrote:

> @@ -1650,6 +1689,7 @@ static int neigh_delete(struct sk_buff *skb, struct nlmsghdr *nlh,
>  			   NEIGH_UPDATE_F_ADMIN,
>  			   NETLINK_CB(skb).portid);
>  	neigh_release(neigh);
> +	neigh_remove_one(neigh, tbl);

	So, we do not hold reference to neigh while accessing
its fields. I suspect we need to move the table lock from
neigh_remove_one here, for example:

+	write_lock_bh(&tbl->lock);
	neigh_release(neigh);
+	neigh_remove_one(neigh, tbl);
+	write_unlock_bh(&tbl->lock);

	Otherwise, neigh_forced_gc can call neigh_destroy
where neigh is freed (and replaced by another neigh entry) after
RCU grace period (which can end prematurely if there are no
running RCU read-side critical sections) and I'm not sure if
our thread can be delayed, so that such pointer replacement
can happen unnoticed by us.

	Another solution to cause faster removal would be
to cancel the gc_work and to schedule it after 1 jiffie.
It helps when many entries are deleted at once but the
work prefers to just sleep when gc_thresh1 is not reached,
so such solution is not good enough.

Regards

--
Julian Anastasov <ja@....bg>