netdev - Re: [PATCH net] ipvlan: add ipvlan_route_v6

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Thu, 23 Nov 2023 18:48:11 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Eric Dumazet <edumazet@...gle.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: "David S . Miller" <davem@...emloft.net>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>, 
 netdev@...r.kernel.org, 
 eric.dumazet@...il.com, 
 syzbot <syzkaller@...glegroups.com>, 
 Mahesh Bandewar <maheshb@...gle.com>, 
 Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net] ipvlan: add ipvlan_route_v6_outbound() helper

Eric Dumazet wrote:
> On Thu, Nov 9, 2023 at 7:29 PM Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
> 
> > Do you think that it is an oversight that this function mixes a return
> > of NET_XMIT_DROP/NET_XMIT_SUCCESS with returning the error code
> > received from deep in the routing stack?
> >
> > Either way, this patch preserves that existing behavior, so
> >
> > Reviewed-by: Willem de Bruijn <willemb@...gle.com>
> 
> I saw this indeed, and chose to leave this as is to ease code review.
> 
> We might send a stand alone patch to return NET_XMIT_DROP instead.
> 
> Thanks for the review !

I took a closer look at this. The short version is that returning
errno, including this dst->error, from ipvlan_start_xmit is apparently
valid, to my surprise. If so, there is nothing to do here.

The various callees of ipvlan_start_xmit return a mixture of
NET_XMIT_SUCCESS, NET_XMIT_DROP, errno (as in this instance, and
whatever __dev_queue_xmit may return, which is documented as

   Return:      
   * 0                          - buffer successfully transmitted
   * positive qdisc return code - NET_XMIT_DROP etc.
   * negative errno             - other errors

Since in all cases the skb is consumed, I thought a simple fix for
this driver might be:

    @@ -232,7 +232,7 @@ static netdev_tx_t ipvlan_start_xmit(struct sk_buff *skb,
            } else {
                    this_cpu_inc(ipvlan->pcpu_stats->tx_drps);
            }
    -       return ret;
    +       return NETDEV_TX_OK;

Given that the return type of ndo_start_xmit is pretty clear:

    enum netdev_tx {
            __NETDEV_TX_MIN  = INT_MIN,     /* make sure enum is signed */
            NETDEV_TX_OK     = 0x00,        /* driver took care of packet */
            NETDEV_TX_BUSY   = 0x10,        /* driver tx path was busy*/
    };
    typedef enum netdev_tx netdev_tx_t;

But, the comment above that in netdevice.h states that drivers may
indeed return other values. There is no strict type checking for enums.

    /*
     * Transmit return codes: transmit return codes originate from three different
     * namespaces:
     *
     * - qdisc return codes
     * - driver transmit return codes
     * - errno values
     *
     * Drivers are allowed to return any one of those in their hard_start_xmit()
     * function. Real network devices commonly used with qdiscs should only return
     * the driver transmit return codes though - when qdiscs are used, the actual
     * transmission happens asynchronously, so the value is not propagated to
     * higher layers. Virtual network devices transmit synchronously; in this case
     * the driver transmit return codes are consumed by dev_queue_xmit(), and all
     * others are propagated to higher layers.
     */

The "real network devices" part is implemented by dev_xmit_complete:

    /*
     * Current order: NETDEV_TX_MASK > NET_XMIT_MASK >= 0 is significant;
     * hard_start_xmit() return < NET_XMIT_MASK means skb was consumed.
     */     
    static inline bool dev_xmit_complete(int rc)
    {       
    	/*
    	 * Positive cases with an skb consumed by a driver:
    	 * - successful transmission (rc == NETDEV_TX_OK)
    	 * - error while transmitting (rc < 0)
    	 * - error while queueing to a different device (rc & NET_XMIT_MASK)
    	 */                      
    	if (likely(rc < NET_XMIT_MASK))
    		return true;

    	return false;
    }               

Note that NETDEV_TX_BUSY is > NET_XMIT_MASK.

I suppose the second part refers to the if (q->enqueue) == false fall-through
in __dev_queue_xmit, where __dev_queue_xmit indeed returns rc to its caller.