Re: [patch net-next V2] net: introduce ethernet teaming device

[Eric Dumazet <eric.dumazet@gmail.com>]

Le dimanche 23 octobre 2011 à 10:25 +0200, Jiri Pirko a écrit :
> Sat, Oct 22, 2011 at 06:51:22PM CEST, eric.dumazet@gmail.com wrote:
> >Le samedi 22 octobre 2011 à 17:13 +0200, Jiri Pirko a écrit :
> >> >> +
> >> >> +/************************
> >> >> + * Rx path frame handler
> >> >> + ************************/
> >> >> +
> >> >> +/* note: already called with rcu_read_lock */
> >> >> +static rx_handler_result_t team_handle_frame(struct sk_buff **pskb)
> >> >> +{
> >> >> +	struct sk_buff *skb = *pskb;
> >> >> +	struct team_port *port;
> >> >> +	struct team *team;
> >> >> +	rx_handler_result_t res = RX_HANDLER_ANOTHER;
> >> >> +
> >> >> +	skb = skb_share_check(skb, GFP_ATOMIC);
> >> >> +	if (!skb)
> >> >> +		return RX_HANDLER_CONSUMED;
> >> >> +
> >> >> +	*pskb = skb;
> >> >> +
> >> >> +	port = team_port_get_rcu(skb->dev);
> >> >> +	team = port->team;
> >> >> +
> >> >> +	if (team->mode_ops.receive)
> >> >
> >> >Hmm, you need ACCESS_ONCE() here or rcu_dereference()
> >> >
> >> >See commit 4d97480b1806e883eb (bonding: use local function pointer of
> >> >bond->recv_probe in bond_handle_frame) for reference
> >> 
> >> I do not think so. Because mode_ops.receive changes only from
> >> __team_change_mode() and this can be called only in case no ports are in
> >> team. And team_port_del() calls synchronize_rcu().
> >> 
> >
> >
> >
> >We are used to code following this template :
> >
> >if (ops->handler)
> >	ops->handler(arguments);
> >
> >But this is valid only because ops points to constant memory.
> >
> >
> >In your case, we really see its not true, dont try to pretend its safe.
> 
> Please forgive me, it's possible I'm missing something. But I see no way how
> team->mode_ops.receive can change during team_handle_frame (holding
> rcu_read_lock) for the reason I stated earlier.
> 
> team_port_del() calls netdev_rx_handler_unregister() and after that it
> calls synchronize_rcu(). That ensures that by the finish of team_port_del()
> run, team_handle_frame() is not called for this port anymore.
> 
> And this combined with "if (!list_empty(&team->port_list))" check in
> team_change_mode() ensures safety.
> 
> Of course team_port_del() and team_change_mode() are both protected by
> team->lock so they are mutually excluded.

Then, why even testing (team->mode_ops.receive) being NULL at the first
place, if you are sure no packets can flight meeting this NULL pointer ?

Something is flawed in the logic.