From owner-svn-src-all@FreeBSD.ORG  Sat May 21 22:26:37 2011
Return-Path: <owner-svn-src-all@FreeBSD.ORG>
Delivered-To: svn-src-all@freebsd.org
Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34])
	by hub.freebsd.org (Postfix) with ESMTP id 43B8E106564A;
	Sat, 21 May 2011 22:26:37 +0000 (UTC)
	(envelope-from rwatson@FreeBSD.org)
Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42])
	by mx1.freebsd.org (Postfix) with ESMTP id 1FD618FC0C;
	Sat, 21 May 2011 22:26:37 +0000 (UTC)
Received: from fledge.watson.org (fledge.watson.org [65.122.17.41])
	by cyrus.watson.org (Postfix) with ESMTPS id CA05646B06;
	Sat, 21 May 2011 18:26:36 -0400 (EDT)
Date: Sat, 21 May 2011 23:26:36 +0100 (BST)
From: Robert Watson <rwatson@FreeBSD.org>
X-X-Sender: robert@fledge.watson.org
To: Luigi Rizzo <luigi@FreeBSD.org>
In-Reply-To: <201011121305.oACD5Htm009484@svn.freebsd.org>
Message-ID: <alpine.BSF.2.00.1105212323450.35370@fledge.watson.org>
References: <201011121305.oACD5Htm009484@svn.freebsd.org>
User-Agent: Alpine 2.00 (BSF 1167 2008-08-23)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed
Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org,
	src-committers@freebsd.org
Subject: Re: svn commit: r215179 - in head: sbin/ipfw sys/netinet
 sys/netinet/ipfw
X-BeenThere: svn-src-all@freebsd.org
X-Mailman-Version: 2.1.5
Precedence: list
List-Id: "SVN commit messages for the entire src tree \(except for &quot;
	user&quot; and &quot; projects&quot; \)" <svn-src-all.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-all>,
	<mailto:svn-src-all-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-all>
List-Post: <mailto:svn-src-all@freebsd.org>
List-Help: <mailto:svn-src-all-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/svn-src-all>,
	<mailto:svn-src-all-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Sat, 21 May 2011 22:26:37 -0000

On Fri, 12 Nov 2010, Luigi Rizzo wrote:

> --- head/sys/netinet/ipfw/ip_fw2.c	Fri Nov 12 13:02:26 2010	(r215178)
> +++ head/sys/netinet/ipfw/ip_fw2.c	Fri Nov 12 13:05:17 2010	(r215179)
> @@ -1801,6 +1801,39 @@ do {								\
...
> +				/* For incomming packet, lookup up the
> +				inpcb using the src/dest ip/port tuple */
> +				if (inp == NULL) {
> +					INP_INFO_RLOCK(pi);
> +					inp = in_pcblookup_hash(pi,
> +						src_ip, htons(src_port),
> +						dst_ip, htons(dst_port),
> +						0, NULL);
> +					INP_INFO_RUNLOCK(pi);
> +				}
> +
> +				if (inp && inp->inp_socket) {
> +					tablearg = inp->inp_socket->so_user_cookie;
> +					if (tablearg)
> +						match = 1;
> +				}

This locking seems questionable -- what keeps 'inp' valid between 
INP_INFO_RUNLOCK(pi) and dereferencing 'inp' a few lines later to extract 
'tablearg'?  Normally, consumer code locks 'inp' after looking it up.

(In my new version of the code, the caller can request it be returned locked, 
or simply referenced, which changes the function signature, hence my bumping 
into this while doing a merge forward).

Robert