[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Sun, 23 Oct 2011 10:43:57 +0200
From: Eric Dumazet <eric.dumazet@...il.com>
To: Jiri Pirko <jpirko@...hat.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net,
bhutchings@...arflare.com, shemminger@...tta.com, fubar@...ibm.com,
andy@...yhouse.net, tgraf@...radead.org, ebiederm@...ssion.com,
mirqus@...il.com, kaber@...sh.net, greearb@...delatech.com,
jesse@...ira.com, fbl@...hat.com, benjamin.poirier@...il.com,
jzupka@...hat.com
Subject: Re: [patch net-next V2] net: introduce ethernet teaming device
Le dimanche 23 octobre 2011 à 10:25 +0200, Jiri Pirko a écrit :
> Sat, Oct 22, 2011 at 06:51:22PM CEST, eric.dumazet@...il.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.
--
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
Powered by blists - more mailing lists