Re: [patch]apmd ? sign
> > But why is this necessary, haven't seen this in other deamons? > > BTW: isn't the \* FALLTHROUGH *\ comment missing? > > It is an old style of coding to allow -? in that way. > It is bogus. I mopped up much of the tree (I think around 2005?) > but there are more opportunities. Please someone do a comprehensive > job.. pcregrep -rM 'case.*:\s*default:' /usr/src finds 795 instances of that pattern. pcregrep is in devel/pcre if you don't already have it. -Matthew Martin
Re: mismatch for ICMP state created by inound response
On Tue, May 19, 2015 at 14:07 +0200, Alexandr Nedvedicky wrote:
> Hello Mike,
>
> I've reworked patch from yesterday. I've done some quick testing
> to see if it fixes problem. It looks like it works. I have not
> tested NAT-64 yet. Also I'd like to come up with test case, which
> will show the state check is still able to block invalid ICMP packet
> (invalid with respect to state).
>
> The idea of fix is to keep icmp_dir in state as well. The icmp_dir
> indicates whether state got created by ICMP request or response.
> This is useful later in pf_icmp_state_lookup() to check whether
> ICMP request/response matches state direction.
>
This feels slightly convoluted... check my diff out! (:
> The attached patch keeps ICMP state match for both cases:
>
> pass in on dst-nic from any to any
>
> and
> pass out on dst-nic from any to any
>
> the dst-nic is NIC towards ICMP destination network.
>
> regards
> sasha
>
> P.S. I took discussion off-line not to create extra noise on tech@openbsd.org
> feel free go get the alias back to loop.
Nah, that's what tech@ is for!
diff --git sys/net/pf.c sys/net/pf.c
index 39d5cb6..81e23de 100644
--- sys/net/pf.c
+++ sys/net/pf.c
@@ -177,11 +177,11 @@ intpf_test_rule(struct pf_pdesc
*, struct pf_rule **,
static __inline int pf_create_state(struct pf_pdesc *, struct pf_rule *,
struct pf_rule *, struct pf_rule *,
struct pf_state_key **, struct pf_state_key **,
int *, struct pf_state **, int,
struct pf_rule_slist *, struct pf_rule_actions *,
- struct pf_src_node *[]);
+ struct pf_src_node *[], int);
static __inline int pf_state_key_addr_setup(struct pf_pdesc *, void *,
int, struct pf_addr *, int, struct pf_addr *,
int, int);
int pf_state_key_setup(struct pf_pdesc *, struct
pf_state_key **, struct pf_state_key **, int);
@@ -3365,11 +3365,11 @@ pf_test_rule(struct pf_pdesc *pd, struct pf_rule **rm,
struct pf_state **sm,
REASON_SET(&reason, PFRES_MAXSTATES);
goto cleanup;
}
action = pf_create_state(pd, r, a, nr, &skw, &sks, &rewrite,
- sm, tag, &rules, &act, sns);
+ sm, tag, &rules, &act, sns, icmp_dir);
if (action != PF_PASS)
return (action);
if (sks != skw) {
struct pf_state_key *sk;
@@ -3433,11 +3433,12 @@ cleanup:
static __inline int
pf_create_state(struct pf_pdesc *pd, struct pf_rule *r, struct pf_rule *a,
struct pf_rule *nr, struct pf_state_key **skw, struct pf_state_key **sks,
int *rewrite, struct pf_state **sm, int tag, struct pf_rule_slist *rules,
-struct pf_rule_actions *act, struct pf_src_node *sns[PF_SN_MAX])
+struct pf_rule_actions *act, struct pf_src_node *sns[PF_SN_MAX],
+int icmp_dir)
{
struct pf_state *s = NULL;
struct tcphdr *th = pd->hdr.tcp;
u_int16_tmss = tcp_mssdflt;
u_short reason;
@@ -3446,10 +3447,11 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r,
struct pf_rule *a,
s = pool_get(&pf_state_pl, PR_NOWAIT | PR_ZERO);
if (s == NULL) {
REASON_SET(&reason, PFRES_MEMORY);
goto csfailed;
}
+ s->direction = pd->dir;
s->rule.ptr = r;
s->anchor.ptr = a;
s->natrule.ptr = nr;
memcpy(&s->match_rules, rules, sizeof(s->match_rules));
STATE_INC_COUNTERS(s);
@@ -3517,10 +3519,14 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r,
struct pf_rule *a,
break;
case IPPROTO_ICMP:
#ifdef INET6
case IPPROTO_ICMPV6:
#endif
+ /* XOR Magic! */
+ s->direction = s->direction == icmp_dir ?
+ PF_IN : PF_OUT;
+
s->timeout = PFTM_ICMP_FIRST_PACKET;
break;
default:
s->src.state = PFOTHERS_SINGLE;
s->dst.state = PFOTHERS_NO_TRAFFIC;
@@ -3543,11 +3549,10 @@ pf_create_state(struct pf_pdesc *pd, struct pf_rule *r,
struct pf_rule *a,
DPFPRINTF(LOG_ERR,
"pf_normalize_tcp_stateful failed on first pkt");
goto csfailed;
}
}
- s->direction = pd->dir;
if (pf_state_key_setup(pd, skw, sks, act->rtableid)) {
REASON_SET(&reason, PFRES_MEMORY);
goto csfailed;
}
Re: [patch]apmd ? sign
On Wed, May 20, 2015 at 10:21:12PM +0200, Fritjof Bornebusch wrote:
> On Wed, May 20, 2015 at 09:35:03PM +0200, Alexander Hall wrote:
> >
> >
> > On May 20, 2015 5:08:21 PM GMT+02:00, Fritjof Bornebusch
> > wrote:
> > >Hi,
> > >
> > >for what is the ? sign for?
> >
> > fallthrough to usage()
> >
>
> But why is this necessary, haven't seen this in other deamons?
> BTW: isn't the \* FALLTHROUGH *\ comment missing?
default: covers the '?' case so it is not stricktly needed but read the
RETURN VALUES section of getopt(3) if you wonder why '?' needs to be
catched.
FALLTHROUGH is only needed if there is code between the case statement as
indication that the break was left out for a reason. In this case it is
obvious that this is a combined case statement.
--
:wq Claudio
> > # apmd -?
> >
> > /Alexander
> >
>
> --F.
>
> > >
> > >Regards,
> > >--F.
> > >
> > >
> > >Index: apmd.c
> > >===
> > >RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
> > >retrieving revision 1.75
> > >diff -u -p -r1.75 apmd.c
> > >--- apmd.c 6 Feb 2015 08:16:50 - 1.75
> > >+++ apmd.c 20 May 2015 15:04:38 -
> > >@@ -403,7 +403,6 @@ main(int argc, char *argv[])
> > > doperf = PERF_MANUAL;
> > > setperfpolicy("high");
> > > break;
> > >- case '?':
> > > default:
> > > usage();
> > > }
> >
Re: [patch]apmd ? sign
> But why is this necessary, haven't seen this in other deamons? > BTW: isn't the \* FALLTHROUGH *\ comment missing? It is an old style of coding to allow -? in that way. It is bogus. I mopped up much of the tree (I think around 2005?) but there are more opportunities. Please someone do a comprehensive job..
Re: [patch]apmd ? sign
On Wed, May 20, 2015 at 09:35:03PM +0200, Alexander Hall wrote:
>
>
> On May 20, 2015 5:08:21 PM GMT+02:00, Fritjof Bornebusch
> wrote:
> >Hi,
> >
> >for what is the ? sign for?
>
> fallthrough to usage()
>
But why is this necessary, haven't seen this in other deamons?
BTW: isn't the \* FALLTHROUGH *\ comment missing?
> # apmd -?
>
> /Alexander
>
--F.
> >
> >Regards,
> >--F.
> >
> >
> >Index: apmd.c
> >===
> >RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
> >retrieving revision 1.75
> >diff -u -p -r1.75 apmd.c
> >--- apmd.c 6 Feb 2015 08:16:50 - 1.75
> >+++ apmd.c 20 May 2015 15:04:38 -
> >@@ -403,7 +403,6 @@ main(int argc, char *argv[])
> > doperf = PERF_MANUAL;
> > setperfpolicy("high");
> > break;
> >-case '?':
> > default:
> > usage();
> > }
>
pgptou0PoyQly.pgp
Description: PGP signature
Re: [patch]apmd ? sign
On May 20, 2015 5:08:21 PM GMT+02:00, Fritjof Bornebusch
wrote:
>Hi,
>
>for what is the ? sign for?
fallthrough to usage()
# apmd -?
/Alexander
>
>Regards,
>--F.
>
>
>Index: apmd.c
>===
>RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
>retrieving revision 1.75
>diff -u -p -r1.75 apmd.c
>--- apmd.c 6 Feb 2015 08:16:50 - 1.75
>+++ apmd.c 20 May 2015 15:04:38 -
>@@ -403,7 +403,6 @@ main(int argc, char *argv[])
> doperf = PERF_MANUAL;
> setperfpolicy("high");
> break;
>- case '?':
> default:
> usage();
> }
[patch]apmd ? sign
Hi,
for what is the ? sign for?
Regards,
--F.
Index: apmd.c
===
RCS file: /cvs/src/usr.sbin/apmd/apmd.c,v
retrieving revision 1.75
diff -u -p -r1.75 apmd.c
--- apmd.c 6 Feb 2015 08:16:50 - 1.75
+++ apmd.c 20 May 2015 15:04:38 -
@@ -403,7 +403,6 @@ main(int argc, char *argv[])
doperf = PERF_MANUAL;
setperfpolicy("high");
break;
- case '?':
default:
usage();
}
pgp1bgAfQehas.pgp
Description: PGP signature
Re: ospfd announces carp interface with physical link down
On Wed, 20 May 2015 12:51:53 +0200
Martin Pieuchot wrote:
>
> > just for completeness: LINK_STATE_INVALID is 1, and that's what
> > carp_set_state uses for everything but master and backup. so far so
> > good.
> >
> > ifp is part of the sc which in turn is malloc'd with M_ZERO in
> > carp_clone_create, so link state will be 0 aka LINK_STATE_UNKNOWN.
> >
> > however, at the end of carp_clone_create, we call
> > carp_set_state_all(sc, INIT) which should take care of that.
>
> Sadly it does not, because of:
>
> 2305: if (vhe->state == state)
> 2306: return;
>
> I'd extend your diff a little bit to make this vhe->state transition
> less confusing, see below. Do you confirm this also fixes your issue?
>
The patch did not apply cleanly to OPENBSD_5_7, I rewrote the patch a
bit:
Index: netinet/ip_carp.c
===
RCS file: /cvs/src/sys/netinet/ip_carp.c,v
retrieving revision 1.247
diff -u -p -r1.247 ip_carp.c
--- netinet/ip_carp.c 4 Mar 2015 10:59:52 - 1.247
+++ netinet/ip_carp.c 20 May 2015 11:53:08 -
@@ -709,9 +709,7 @@ carpattach(int n)
}
int
-carp_clone_create(ifc, unit)
- struct if_clone *ifc;
- int unit;
+carp_clone_create(struct if_clone *ifc, int unit)
{
struct carp_softc *sc;
struct ifnet *ifp;
@@ -751,6 +749,7 @@ carp_clone_create(ifc, unit)
ifp->if_addrlen = ETHER_ADDR_LEN;
ifp->if_hdrlen = ETHER_HDR_LEN;
ifp->if_mtu = ETHERMTU;
+ ifp->if_link_state = LINK_STATE_INVALID;
IFQ_SET_MAXLEN(&ifp->if_snd, IFQ_MAXLEN);
IFQ_SET_READY(&ifp->if_snd);
if_attach(ifp);
@@ -764,7 +763,6 @@ carp_clone_create(ifc, unit)
/* Hook carp_addr_updated to cope with address and route
changes. */ sc->ah_cookie = hook_establish(sc->sc_if.if_addrhooks, 0,
carp_addr_updated, sc);
- carp_set_state_all(sc, INIT);
return (0);
}
@@ -781,6 +779,7 @@ carp_new_vhost(struct carp_softc *sc, in
vhe->parent_sc = sc;
vhe->vhid = vhid;
vhe->advskew = advskew;
+ vhe->state = INIT;
timeout_set(&vhe->ad_tmo, carp_send_ad, vhe);
timeout_set(&vhe->md_tmo, carp_master_down, vhe);
timeout_set(&vhe->md6_tmo, carp_master_down, vhe);
@@ -2288,8 +2287,12 @@ carp_set_state_all(struct carp_softc *sc
{
struct carp_vhost_entry *vhe;
- LIST_FOREACH(vhe, &sc->carp_vhosts, vhost_entries)
+ LIST_FOREACH(vhe, &sc->carp_vhosts, vhost_entries) {
+ if (vhe->state == state)
+ continue;
+
carp_set_state(vhe, state);
+ }
}
void
@@ -2299,8 +2302,8 @@ carp_set_state(struct carp_vhost_entry *
static const char *carp_states[] = { CARP_STATES };
int loglevel;
- if (vhe->state == state)
- return;
+ KASSERT(vhe->state != state);
+
if (vhe->state == INIT || state == INIT)
loglevel = LOG_WARNING;
else
With this patch everything (almost) work. At least as good as my patch
did. OSPFd still does something wrong with the link state of carp
interfaces when starting. Have a look at this:
fw2:/usr/src/sys # ospfctl show int
Interface AddressState HelloTimer Linkstate Uptimenc ac
carp7 195.58.98.145/28 DOWN - backup 00:00:00 0 0
carp5 192.168.253.1/24 DOWN - backup 00:00:00 0 0
carp3 192.168.202.1/24 DOWN - backup 00:00:00 0 0
carp2 192.168.254.1/23 DOWN - invalid00:00:00 0 0
carp1 31.15.61.129/26DOWN - invalid00:00:00 0 0
carp0 92.33.0.202/30 DOWN - backup 00:00:00 0 0
bnx0192.168.200.5/24 OTHER 00:00:00 active 00:13:13 4 2
carp2 is (correctly) invalid, because the cable is plugged.
carp1 is _not_ invalid. If I restart ospfd after the system has come up it
looks better:
carp1 31.15.61.129/26DOWN - backup 00:00:00 0 0
This happens with random interfaces at start-up.
I believe this may be the cause:
in usr.sbin/ospfd/interface.c, if_act_start():
if (!((iface->flags & IFF_UP) &&
LINK_STATE_IS_UP(iface->linkstate)))
return (0);
This check lack the exception for carp interfaces found in ospfe.c. If
the interface already has been initialized when ospfd starts, it will
not pick that interface up as a carp interface.
/Johan
Re: ospfd announces carp interface with physical link down
On 20/05/15(Wed) 07:40, Henning Brauer wrote:
> * Johan Ymerson [2015-05-19 19:25]:
> > On Tue, 2015-05-19 at 11:16 +, Johan Ymerson wrote:
> > > On Tue, 2015-05-19 at 11:24 +0100, Stuart Henderson wrote:
> > > > On 2015/05/19 10:10, Johan Ymerson wrote:
> > > > Yes I understand that, but if carp init was counted in "LINK_STATE_DOWN"
> > > > then the metric would be 65535 which I think would still avoid the
> > > > problem you're seeing, and would involve less special-casing in ospfd.
> > > Yes, that would also resolve the issue, but it is a bit illogical to
> > > announce a network we cannot possibly route traffic to (due to hardware
> > > problems).
> > After some more testing I think we can conclude that this is most
> > definitely a kernel issue.
>
> hmm. there's definately more to it.
Indeed
> just for completeness: LINK_STATE_INVALID is 1, and that's what
> carp_set_state uses for everything but master and backup. so far so
> good.
>
> ifp is part of the sc which in turn is malloc'd with M_ZERO in
> carp_clone_create, so link state will be 0 aka LINK_STATE_UNKNOWN.
>
> however, at the end of carp_clone_create, we call
> carp_set_state_all(sc, INIT) which should take care of that.
Sadly it does not, because of:
2305: if (vhe->state == state)
2306: return;
I'd extend your diff a little bit to make this vhe->state transition
less confusing, see below. Do you confirm this also fixes your issue?
Index: netinet/ip_carp.c
===
RCS file: /cvs/src/sys/netinet/ip_carp.c,v
retrieving revision 1.256
diff -u -p -r1.256 ip_carp.c
--- netinet/ip_carp.c 15 May 2015 11:53:06 - 1.256
+++ netinet/ip_carp.c 20 May 2015 10:48:17 -
@@ -708,9 +708,7 @@ carpattach(int n)
}
int
-carp_clone_create(ifc, unit)
- struct if_clone *ifc;
- int unit;
+carp_clone_create(struct if_clone *ifc, int unit)
{
struct carp_softc *sc;
struct ifnet *ifp;
@@ -753,11 +751,11 @@ carp_clone_create(ifc, unit)
ifp->if_sadl->sdl_type = IFT_CARP;
ifp->if_output = carp_output;
ifp->if_priority = IF_CARP_DEFAULT_PRIORITY;
+ ifp->if_link_state = LINK_STATE_INVALID;
/* Hook carp_addr_updated to cope with address and route changes. */
sc->ah_cookie = hook_establish(sc->sc_if.if_addrhooks, 0,
carp_addr_updated, sc);
- carp_set_state_all(sc, INIT);
return (0);
}
@@ -774,6 +772,7 @@ carp_new_vhost(struct carp_softc *sc, in
vhe->parent_sc = sc;
vhe->vhid = vhid;
vhe->advskew = advskew;
+ vhe->state = INIT;
timeout_set(&vhe->ad_tmo, carp_send_ad, vhe);
timeout_set(&vhe->md_tmo, carp_master_down, vhe);
timeout_set(&vhe->md6_tmo, carp_master_down, vhe);
@@ -2276,8 +2275,12 @@ carp_set_state_all(struct carp_softc *sc
{
struct carp_vhost_entry *vhe;
- LIST_FOREACH(vhe, &sc->carp_vhosts, vhost_entries)
+ LIST_FOREACH(vhe, &sc->carp_vhosts, vhost_entries) {
+ if (vhe->state == state)
+ continue;
+
carp_set_state(vhe, state);
+ }
}
void
@@ -2287,8 +2290,8 @@ carp_set_state(struct carp_vhost_entry *
static const char *carp_states[] = { CARP_STATES };
int loglevel;
- if (vhe->state == state)
- return;
+ KASSERT(vhe->state != state);
+
if (vhe->state == INIT || state == INIT)
loglevel = LOG_WARNING;
else
Re: [patch]rcs: xstrdup just wrappes strdup
On Tue, May 19, 2015 at 08:57:06PM +0200, Fritjof Bornebusch wrote:
> Hi,
>
> xstrdup just wrappes strdup, so there is no need to call xmalloc and
> strlcpy instead.
>
Use err() instead of errx(), so errno will be printed additionally.
Thanks to Tim.
> Regards,
> --F.
>
>
Regards,
--F.
>
Index: xmalloc.c
===
RCS file: /cvs/src/usr.bin/rcs/xmalloc.c,v
retrieving revision 1.8
diff -u -p -r1.8 xmalloc.c
--- xmalloc.c 26 Mar 2015 15:17:30 - 1.8
+++ xmalloc.c 20 May 2015 08:53:01 -
@@ -76,13 +76,11 @@ xfree(void *ptr)
char *
xstrdup(const char *str)
{
- size_t len;
char *cp;
- len = strlen(str) + 1;
- cp = xmalloc(len);
- if (strlcpy(cp, str, len) >= len)
- errx(1, "xstrdup: string truncated");
+ if ((cp = strdup(str)) == NULL)
+ err(1, "xstrdup: copy string failed");
+
return cp;
}
pgpW87xQmqoAq.pgp
Description: PGP signature
