[ofa-general] [PATCH] IPoIB: check multicast address format

[Moni Shoua]

Jason Gunthorpe wrote:
> Check that the format of the multicast link address is correct before
> taking it from dev->mc_list to priv->multicast_list. This way we never
> try to send a bogus address to the SA, and prevents badness from
> erronous 'ip maddr addr add', broken bonding drivers, or whatever.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe at obsidianresearch.com>
> ---
>  drivers/infiniband/ulp/ipoib/ipoib_multicast.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> Same problem Moni was working on, but lets just address it directly.
> 
> There is work to try and fix the bonding driver but no fixed version
> is in mainline yet. This is a cheap and simple work around that is
> worth having even once the driver is fixed.
> 
> Despite this, I think it is still necessary to do something like Moni
> was trying - to prevent the MCG join queue from head of line blocking
> on a single bad SA response. This can happen even if everything is
> correct.
> 
> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> index 425e311..973a24b 100644
> --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
> @@ -758,6 +758,20 @@ void ipoib_mcast_dev_flush(struct net_device *dev)
>  	}
>  }
>  
> +static int check_mcast(const u8 *addr,unsigned int addrlen,
> +		       const u8 *broadcast)
> +{
> +	if (addrlen != 20)
> +		return 0;
> +	/* QPN, scope, reserved, sigature upper */
> +	if (memcmp(addr,broadcast,6) != 0)
> +		return 0;
> +	/* signature lower, pkey */
> +	if (memcmp(addr + 7,broadcast+7,3) != 0)
> +		return 0;
> +	return 1;
> +}
> +
>  void ipoib_mcast_restart_task(struct work_struct *work)
>  {
>  	struct ipoib_dev_priv *priv =
> @@ -791,6 +805,10 @@ void ipoib_mcast_restart_task(struct work_struct *work)
>  	for (mclist = dev->mc_list; mclist; mclist = mclist->next) {
>  		union ib_gid mgid;
>  
> +		if (!check_mcast(mclist->dmi_addr,mclist->dmi_addrlen,
> +				 dev->broadcast))
> +			continue;
> +
>  		memcpy(mgid.raw, mclist->dmi_addr + 4, sizeof mgid);
>  
>  		mcast = __ipoib_mcast_find(dev, &mgid);

Why not check validity with something that looks like the reverse operation 
of the kernel function that maps ip -> link mcast addresses? 

for example this is the function for IPv4

static inline void ip_ib_mc_map(__be32 naddr, const unsigned char *broadcast, char *buf)
{
        __u32 addr;
        unsigned char scope = broadcast[5] & 0xF;

        buf[0]  = 0;            /* Reserved */
        buf[1]  = 0xff;         /* Multicast QPN */
        buf[2]  = 0xff;
        buf[3]  = 0xff;
        addr    = ntohl(naddr);
        buf[4]  = 0xff;
        buf[5]  = 0x10 | scope; /* scope from broadcast address */
        buf[6]  = 0x40;         /* IPv4 signature */
        buf[7]  = 0x1b;
        buf[8]  = broadcast[8];         /* P_Key */
        buf[9]  = broadcast[9];
        buf[10] = 0;
        buf[11] = 0;
        buf[12] = 0;
        buf[13] = 0;
        buf[14] = 0;
        buf[15] = 0;
        buf[19] = addr & 0xff;
        addr  >>= 8;
        buf[18] = addr & 0xff;
        addr  >>= 8;
        buf[17] = addr & 0xff;
        addr  >>= 8;
        buf[16] = addr & 0x0f;
}