From owner-freebsd-bugs@FreeBSD.ORG Wed Dec 17 09:50:43 2003 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 90A3916A4CE for ; Wed, 17 Dec 2003 09:50:43 -0800 (PST) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id 1AABC43D58 for ; Wed, 17 Dec 2003 09:50:23 -0800 (PST) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) hBHHoMFR009671 for ; Wed, 17 Dec 2003 09:50:22 -0800 (PST) (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.12.10/8.12.10/Submit) id hBHHoMIY009670; Wed, 17 Dec 2003 09:50:22 -0800 (PST) (envelope-from gnats) Date: Wed, 17 Dec 2003 09:50:22 -0800 (PST) Message-Id: <200312171750.hBHHoMIY009670@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Yar Tikhiy Subject: Re: kern/47920: if ng_pppoe switches to nonstandard mode it stays in it forever X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list Reply-To: Yar Tikhiy List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Dec 2003 17:50:43 -0000 The following reply was made to PR kern/47920; it has been noted by GNATS. From: Yar Tikhiy To: Gleb Smirnoff , FreeBSD-gnats-submit@FreeBSD.ORG Cc: Subject: Re: kern/47920: if ng_pppoe switches to nonstandard mode it stays in it forever Date: Wed, 17 Dec 2003 20:42:34 +0300 On Tue, Dec 09, 2003 at 06:20:54PM +0300, Gleb Smirnoff wrote: > On Tue, Dec 09, 2003 at 04:14:41PM +0300, Yar Tikhiy wrote: > Y> I would rather change the name of the variable "nonstandard" > Y> to "pppoe_mode", not "standard", because lines like this one: > Y> > Y> standard = PPPOE_NONSTANDARD; > Y> > Y> give me a schizoid feeling :-) > > Here it is. Would you mind testing the below version of your patch, revised. Please pay attention to its points: a) sanity check values passed through sysctl; b) avoid double setting nonstandard mode; c) format source according to style(9); d) initialize pppoe_mode for clarity since it's no longer just a boolean trigger. -- Yar Index: ng_pppoe.c =================================================================== RCS file: /home/ncvs/src/sys/netgraph/ng_pppoe.c,v retrieving revision 1.58 diff -u -p -r1.58 ng_pppoe.c --- ng_pppoe.c 19 Feb 2003 05:47:31 -0000 1.58 +++ ng_pppoe.c 17 Dec 2003 17:29:58 -0000 @@ -54,6 +54,7 @@ #include #include #include +#include #include #include @@ -244,23 +245,36 @@ struct ether_header eh_prototype = {0x00,0x00,0x00,0x00,0x00,0x00}, ETHERTYPE_PPPOE_DISC}; -static int nonstandard; +#define PPPOE_KEEPSTANDARD -1 /* never switch to nonstandard mode */ +#define PPPOE_STANDARD 0 /* try standard mode (default) */ +#define PPPOE_NONSTANDARD 1 /* just be in nonstandard mode */ +static int pppoe_mode = PPPOE_STANDARD; + static int ngpppoe_set_ethertype(SYSCTL_HANDLER_ARGS) { int error; int val; - val = nonstandard; + val = pppoe_mode; error = sysctl_handle_int(oidp, &val, sizeof(int), req); if (error != 0 || req->newptr == NULL) return (error); - if (val == 1) { - nonstandard = 1; + switch (val) { + case PPPOE_NONSTANDARD: + pppoe_mode = PPPOE_NONSTANDARD; eh_prototype.ether_type = ETHERTYPE_PPPOE_STUPID_DISC; - } else { - nonstandard = 0; + break; + case PPPOE_STANDARD: + pppoe_mode = PPPOE_STANDARD; eh_prototype.ether_type = ETHERTYPE_PPPOE_DISC; + break; + case PPPOE_KEEPSTANDARD: + pppoe_mode = PPPOE_KEEPSTANDARD; + eh_prototype.ether_type = ETHERTYPE_PPPOE_DISC; + break; + default: + return (EINVAL); } return (0); } @@ -982,8 +996,21 @@ AAA length = ntohs(wh->ph.length); switch(wh->eh.ether_type) { case ETHERTYPE_PPPOE_STUPID_DISC: - nonstandard = 1; - eh_prototype.ether_type = ETHERTYPE_PPPOE_STUPID_DISC; + if (pppoe_mode == PPPOE_STANDARD) { + pppoe_mode = PPPOE_NONSTANDARD; + eh_prototype.ether_type = + ETHERTYPE_PPPOE_STUPID_DISC; + log(LOG_NOTICE, + "Switched to nonstandard PPPoE mode due to " + "packet from %*D\n", + sizeof(wh->eh.ether_shost), + wh->eh.ether_shost, ":"); + } else if (pppoe_mode == PPPOE_KEEPSTANDARD) + log(LOG_NOTICE, + "Ignored nonstandard PPPoE packet " + "from %*D\n", + sizeof(wh->eh.ether_shost), + wh->eh.ether_shost, ":"); /* fall through */ case ETHERTYPE_PPPOE_DISC: /* @@ -1185,7 +1212,7 @@ AAA * from NEWCONNECTED to CONNECTED */ sp->pkt_hdr = neg->pkt->pkt_header; - if (nonstandard) + if (pppoe_mode == PPPOE_NONSTANDARD) sp->pkt_hdr.eh.ether_type = ETHERTYPE_PPPOE_STUPID_SESS; else @@ -1237,7 +1264,7 @@ AAA * Keep a copy of the header we will be using. */ sp->pkt_hdr = neg->pkt->pkt_header; - if (nonstandard) + if (pppoe_mode == PPPOE_NONSTANDARD) sp->pkt_hdr.eh.ether_type = ETHERTYPE_PPPOE_STUPID_SESS; else @@ -1519,7 +1546,7 @@ AAA /* revert the stored header to DISC/PADT mode */ wh = &sp->pkt_hdr; wh->ph.code = PADT_CODE; - if (nonstandard) + if (pppoe_mode == PPPOE_NONSTANDARD) wh->eh.ether_type = ETHERTYPE_PPPOE_STUPID_DISC; else wh->eh.ether_type = ETHERTYPE_PPPOE_DISC;