Re: [PATCH net-next v2] net: dsa: Mock-up driver

[Florian Fainelli <f.fainelli@gmail.com>]

Hi Andrew,

On 03/31/2017 09:06 AM, Andrew Lunn wrote:
> Hi Florian
> 
>> +static enum dsa_tag_protocol dsa_loop_get_protocol(struct dsa_switch *ds)
>> +{
>> +	dev_dbg(ds->dev, "%s\n", __func__);
>> +
>> +	return DSA_TAG_PROTO_NONE;
>> +}
> 
> I'm wondering how safe this is:
> 
> static const struct dsa_device_ops none_ops = {
>         .xmit   = dsa_slave_notag_xmit,
>         .rcv    = NULL,
> };
> 
>         /*
>          * If the CPU connects to this switch, set the switch tree
>          * tagging protocol to the preferred tagging format of this
>          * switch.
>          */
>         if (dst->cpu_switch == ds) {
>                 enum dsa_tag_protocol tag_protocol;
> 
>                 tag_protocol = ops->get_tag_protocol(ds);
>                 dst->tag_ops = dsa_resolve_tag_protocol(tag_protocol);
>                 if (IS_ERR(dst->tag_ops))
>                         return PTR_ERR(dst->tag_ops);
> 
>                 dst->rcv = dst->tag_ops->rcv;
>         }
> 
> 
> static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev,
>                           struct packet_type *pt, struct net_device *orig_dev)
> {
>         struct dsa_switch_tree *dst = dev->dsa_ptr;
> 
>         if (unlikely(dst == NULL)) {
>                 kfree_skb(skb);
>                 return 0;
>         }
> 
>         return dst->rcv(skb, dev, pt, orig_dev);
> }
> 
> static struct packet_type dsa_pack_type __read_mostly = {
>         .type   = cpu_to_be16(ETH_P_XDSA),
>         .func   = dsa_switch_rcv,
> };
> 
> It looks like when a frame is received, we are going to dereference a
> NULL pointer.

Actually we do not, because netdev_uses_dsa() returns true only when
dst->rcv is different from NULL. When dst->rcv is NULL we completely
bypass the DSA hook in eth_type_trans() and everything is well, the
master network device is the one receiving packets.

This is actually the intended behavior for netdev_uses_dsa() because it
really tells whether there is a DSA tagging protocol set-up and that is
what NIC drivers (e.g: bcmsysport) would care about.

Thanks!
-- 
Florian