netdev - Re: [net-next (TAKE 2) 3/4] firewire net, ipv4 arp: Extend hardware address and remove driver-level packet inspection.

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20130210151056.3e6e9f53@stein>
Date:	Sun, 10 Feb 2013 15:10:56 +0100
From:	Stefan Richter <stefanr@...6.in-berlin.de>
To:	YOSHIFUJI Hideaki <yoshfuji@...ux-ipv6.org>
Cc:	netdev@...r.kernel.org, linux1394-devel@...ts.sourceforge.net,
	davem@...emloft.net
Subject: Re: [net-next (TAKE 2) 3/4] firewire net, ipv4 arp: Extend hardware
 address and remove driver-level packet inspection.

On Feb 10 YOSHIFUJI Hideaki wrote:
> @@ -1140,18 +1051,6 @@ static int fwnet_broadcast_start(struct fwnet_device *dev)
>  	unsigned long offset;
>  	unsigned u;
>  
> -	if (dev->local_fifo == FWNET_NO_FIFO_ADDR) {
> -		dev->handler.length = 4096;
> -		dev->handler.address_callback = fwnet_receive_packet;
> -		dev->handler.callback_data = dev;
> -
> -		retval = fw_core_add_address_handler(&dev->handler,
> -					&fw_high_memory_region);
> -		if (retval < 0)
> -			goto failed_initial;
> -
> -		dev->local_fifo = dev->handler.offset;
> -	}
>  
>  	max_receive = 1U << (dev->card->max_receive + 1);
>  	num_packets = (FWNET_ISO_PAGE_COUNT * PAGE_SIZE) / max_receive;
> @@ -1234,8 +1133,6 @@ static int fwnet_broadcast_start(struct fwnet_device *dev)
>  	dev->broadcast_rcv_context = NULL;
>   failed_context_create:
>  	fw_core_remove_address_handler(&dev->handler);
> - failed_initial:
> -	dev->local_fifo = FWNET_NO_FIFO_ADDR;
>  
>  	return retval;
>  }

[...]

> @@ -1546,12 +1421,21 @@ static int fwnet_probe(struct device *_dev)
>  	dev->broadcast_rcv_context = NULL;
>  	dev->broadcast_xmt_max_payload = 0;
>  	dev->broadcast_xmt_datagramlabel = 0;
> -	dev->local_fifo = FWNET_NO_FIFO_ADDR;
>  	dev->queued_datagrams = 0;
>  	INIT_LIST_HEAD(&dev->peer_list);
>  	dev->card = card;
>  	dev->netdev = net;
>  
> +	dev->handler.length = 4096;
> +	dev->handler.address_callback = fwnet_receive_packet;
> +	dev->handler.callback_data = dev;
> +
> +	ret = fw_core_add_address_handler(&dev->handler, &fw_high_memory_region);
> +	if (ret < 0)
> +		goto out;
> +
> +	dev->local_fifo = dev->handler.offset;
> +
>  	/*
>  	 * Use the RFC 2734 default 1500 octets or the maximum payload
>  	 * as initial MTU

If at all possible, please put changes of this kind into a separate
patch.

I get a distinct feeling that the error handling paths in
fwnet_broadcast_start() and in fwnet_probe() respectively became buggy now.
This would be easier to verify if shifting this code was done in a separate
change.

On a loosely related note:
Current firewire-net should be changed to release the isochronous
reception context (the DMA context which is used for broadcast and unicast
reception) at ifdown.  That would make it symmetrical to its allocation
(at ifup) and, importantly, would be the earliest opportunity to release
it.  This is important because isochronous reception DMA contexts are a
scarce hardware resource.
-- 
Stefan Richter
-=====-===-= --=- -=-=-
http://arcgraph.de/sr/
--
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