From owner-freebsd-net@FreeBSD.ORG Tue May 17 18:05:19 2005 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 0FC0E16A4CE for ; Tue, 17 May 2005 18:05:19 +0000 (GMT) Received: from mail-relay1.yahoo.com (mail-relay1.yahoo.com [216.145.48.34]) by mx1.FreeBSD.org (Postfix) with ESMTP id A2A8A43D67 for ; Tue, 17 May 2005 18:05:18 +0000 (GMT) (envelope-from gnn@neville-neil.com) Received: from minion.local.neville-neil.com (proxy7.corp.yahoo.com [216.145.48.98])j4HI5AT4089858; Tue, 17 May 2005 11:05:10 -0700 (PDT) Date: Tue, 17 May 2005 11:05:10 -0700 Message-ID: From: gnn@freebsd.org To: JINMEI Tatuya / =?ISO-2022-JP?B?GyRCP0BMQEMjOkgbKEI=?= In-Reply-To: References: User-Agent: Wanderlust/2.12.0 (Your Wildest Dreams) SEMI/1.14.6 (Maruoka) FLIM/1.14.6 (Marutamachi) APEL/10.6 Emacs/21.3.50 (powerpc-apple-darwin7.7.0) MULE/5.0 (SAKAKI) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII cc: freebsd-net@freebsd.org cc: kame Subject: Re: (KAME-snap 9050) Re: Code nit questions... X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 May 2005 18:05:19 -0000 At Tue, 17 May 2005 23:04:11 +0900, jinmei wrote: > > >>>>> On Thu, 12 May 2005 22:49:12 -0400, > >>>>> gnn@freebsd.org said: > > > In a continuing effort to clean up some code nits in the IPv6 code > > I'd like to propose the following diffs. There is a comment, starting > > with a *) explaining the problem and proposed fix. > > Thanks for your continuous efforts. Here are some comments. No problem, it's my pleasure. > > *) Insert proper return value checking. > > in6_embedscope() should not fail in this context (so we could even > panic if it does), but you probably want to be very proactive by > eliminating as many (hidden) assumptions as possible. If so, please > go ahead with the change, but then I'd rather check the return value > of in6_recoverscope() as well. Checking the return value of in6_recoverscope() seems fine to me if it is also OK by you. If you/kame commit that then I'll try to pick up the change when it happens. > > *) Make sure that sro is also valid before de-referencing it. > > Aside from the fact that sro is not a pointer type as Andre pointed > out, I'm not even sure whether the code around this point is correct > in the first place. Rev. 1.30 of in6_src.c currently reads: This proposed change was dropped, it was my mistake. > > - if (opts && opts->ip6po_pktinfo && > > + if (ifp && opts && opts->ip6po_pktinfo && > > opts-> ip6po_pktinfo->ipi6_ifindex) { > > if (!(ifp->if_flags & IFF_LOOPBACK) && > > ifp->if_index != > > This one does not seem to harm, although ifp can probably be never > NULL in this context as commented around this part (but again, you > probably want to be very proactive, then it's fine). OK, that's been committed to FreeBSD-CURRENT. > > *) Make sure that rule is valid before dereferencing it. > > (I don't know much about the ip6_fw implementation, so my comment is > not based on any expertise of my own as a KAME developer.) > > Although the check does not seem to harm per se, it looks to me that > rule can never be NULL based on the logic of the big for loop above > this point. In fact, the code has an assertion check which detects > almost the same erroneous condition: > > #ifdef DIAGNOSTIC > /* Rule 65535 should always be there and should always match */ > if (!chain) > panic("ip6_fw: chain"); > #endif > > So, there seem to be some inconsistency about unexpected failure. > Hmm, I'll have to look at that again. > > *) Do not bcopy if the pointer is NULL, whether or not canwait was > > set. > > Hmm...can't we assume malloc() returns a valid (non NULL) pointer if > its fourth argument is not M_NOWAIT? If we can (i.e, it's a > function's feature), "ip6po_nexthop == NULL && canwait != M_NOWAIT" > should indicate a serious bug within malloc() or some other parts of > the kernel. So I'm not sure if it's really a good idea to pretend > that the bug didn't exist... > > BTW: if you really want to go with this change, you'll also want to > make the same change on ip6po_pktinfo for consistency. Sigh, that's a good point that neither the tool checking this, nor I, took into account. I will most likely reverse this change and put in a comment. Thanks, George