netdev - Re: [PATCH v3 net-next 3/4] rocker: do not make neighbour entry changes when preparing transactions

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: <20150520074810.GA21934@vergenet.net>
Date:	Wed, 20 May 2015 16:48:13 +0900
From:	Simon Horman <simon.horman@...ronome.com>
To:	Toshiaki Makita <makita.toshiaki@....ntt.co.jp>
Cc:	Scott Feldman <sfeldma@...il.com>, Jiri Pirko <jiri@...nulli.us>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH v3 net-next 3/4] rocker: do not make neighbour entry
 changes when preparing transactions

On Wed, May 20, 2015 at 03:15:23PM +0900, Toshiaki Makita wrote:
> On 2015/05/20 14:48, Simon Horman wrote:
> > rocker_port_ipv4_nh() and in turn rocker_port_ipv4_neigh() may be
> > be called with trans == SWITCHDEV_TRANS_PREPARE and then
> > trans == SWITCHDEV_TRANS_COMMIT from switchdev_port_obj_set() via
> > fib_table_insert().
> > 
> > The first time that rocker_port_ipv4_nh() is called, with
> > trans == SWITCHDEV_TRANS_PREPARE, _rocker_neigh_add() adds a new entry to
> > the neigh table.
> > 
> > And the second time  rocker_port_ipv4_nh() is called, with
> > trans == SWITCHDEV_TRANS_COMMIT, that entry is found. This causes
> > rocker_port_ipv4_nh() to believe it is not adding an entry and thus it
> > frees "entry", which is still present in rocker driver's neigh table.
> > 
> > This problem does not appear to affect deletion as my analysis is that
> > deletion is always performed with trans == SWITCHDEV_TRANS_NONE.
> > 
> > For completeness _rocker_neigh_{add,del,prepare} are updated not to
> > manipulate fib table entries if trans == SWITCHDEV_TRANS_PREPARE.
> > 
> > Fixes: c4f20321d968 ("rocker: support prepare-commit transaction model")
> > Reported-by: oshiaki Makita <makita.toshiaki@....ntt.co.jp>
> 
> 'T' is missing from my first name

Sorry about that.

> > Acked-by: Scott Feldman <sfeldma@...il.com>
> > Signed-off-by: Simon Horman <simon.horman@...ronome.com>
> > 
> ...
> >  static void _rocker_neigh_add(struct rocker *rocker,
> > +			      enum switchdev_trans trans,
> >  			      struct rocker_neigh_tbl_entry *entry)
> >  {
> > +	if (trans == SWITCHDEV_TRANS_PREPARE)
> > +		return;
> >  	entry->index = rocker->neigh_tbl_next_index++;
> 
> Isn't index needed here? It looks to be used in later function call and
> logging.

Thanks, that does not follow the usual model of setting values
during the PREPARE (and all other) transaction phase(s).

> How about setting index like this?
> 
> 	entry->index = rocker->neigh_tbl_next_index;
> 	if (trans == PREPARE)
> 		return;
> 	rocker->neigh_tbl_next_index++;
> 	...

I am concerned that _rocker_neigh_add() may be called by some other
caller while a transaction is in process and thus entry->index will
be inconsistent across callers.

Perhaps we can convince ourselves that all the bases are covered.
So far my testing has drawn a blank. But the logic seems difficult to
reason about.

As we are basically allocating an index I suppose what is really needed for
a correct implementation is a transaction aware index allocator, like we
have for memory (rocker_port_kzalloc etc...).  But that does seem like
overkill.

I think that we can make entry->index consistent across
calls in the same transaction at the expense of breaking the
rule that per-transaction data should be set during all transaction phases.

Something like this:


	if (trans != SWITCHDEV_TRANS_COMMIT)
		/* Avoid index being set to different values across calls
		 * to this function by the same caller within the same
		 * transaction.
		 */
		entry->index = rocker->neigh_tbl_next_index++;
	if (trans == SWITCHDEV_TRANS_PREPARE)
		return;
--
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