rdist/rdistd: use mode_t for file modes

[Todd C. Miller]

The file mode is passed from client to server as a printf string
formatted with %04o (unsigned) so use strtoul() not strtol() to
parse it.  Error out on modes > 0.

There is no way that the mode can ever be -1 so remove those checks.

This rabbit hole brought to you by:
usr.bin/rdistd/server.c:994: warning: comparison between signed and unsigned

 - todd

Index: rdist/client.c
===
RCS file: /cvs/src/usr.bin/rdist/client.c,v
retrieving revision 1.35
diff -u -p -r1.35 client.c
--- rdist/client.c  9 Dec 2015 19:39:10 -   1.35
+++ rdist/client.c  30 Mar 2016 20:52:16 -
@@ -772,8 +772,8 @@ update(char *rname, opt_t opts, struct s
 {
off_t size;
time_t mtime;
-   unsigned short lmode;
-   unsigned short rmode;
+   mode_t lmode;
+   mode_t rmode;
char *owner = NULL, *group = NULL;
int done, n;
u_char *cp;
@@ -1042,7 +1042,7 @@ statupdate(int u, char *starget, opt_t o
 {
int rv = 0;
char ername[PATH_MAX*4];
-   int lmode = st->st_mode & 0;
+   mode_t lmode = st->st_mode & 0;
 
if (u == US_CHMOG) {
if (IS_ON(opts, DO_VERIFY)) {
Index: rdistd/server.c
===
RCS file: /cvs/src/usr.bin/rdistd/server.c,v
retrieving revision 1.42
diff -u -p -r1.42 server.c
--- rdistd/server.c 30 Mar 2016 20:51:59 -  1.42
+++ rdistd/server.c 30 Mar 2016 20:52:16 -
@@ -60,8 +60,8 @@ int   oumask; /* Old umask */
 
 static int cattarget(char *);
 static int setownership(char *, int, uid_t, gid_t, int);
-static int setfilemode(char *, int, int, int);
-static int fchog(int, char *, char *, char *, int);
+static int setfilemode(char *, int, mode_t, int);
+static int fchog(int, char *, char *, char *, mode_t);
 static int removefile(struct stat *, int);
 static void doclean(char *);
 static void clean(char *);
@@ -70,9 +70,9 @@ static void docmdspecial(void);
 static void query(char *);
 static int chkparent(char *, opt_t);
 static char *savetarget(char *, opt_t);
-static void recvfile(char *, opt_t, int, char *, char *, time_t, time_t, 
off_t);
-static void recvdir(opt_t, int, char *, char *);
-static void recvlink(char *, opt_t, int, off_t);
+static void recvfile(char *, opt_t, mode_t, char *, char *, time_t, time_t, 
off_t);
+static void recvdir(opt_t, mode_t, char *, char *);
+static void recvlink(char *, opt_t, mode_t, off_t);
 static void hardlink(char *);
 static void setconfig(char *);
 static void recvit(char *, int);
@@ -148,13 +148,10 @@ setownership(char *file, int fd, uid_t u
  * Set mode of a file
  */
 static int
-setfilemode(char *file, int fd, int mode, int islink)
+setfilemode(char *file, int fd, mode_t mode, int islink)
 {
int status = -1;
 
-   if (mode == -1)
-   return(0);
-
if (islink)
status = fchmodat(AT_FDCWD, file, mode, AT_SYMLINK_NOFOLLOW);
 
@@ -175,7 +172,7 @@ setfilemode(char *file, int fd, int mode
  * Change owner, group and mode of file.
  */
 static int
-fchog(int fd, char *file, char *owner, char *group, int mode)
+fchog(int fd, char *file, char *owner, char *group, mode_t mode)
 {
static struct group *gr = NULL;
int i;
@@ -190,7 +187,7 @@ fchog(int fd, char *file, char *owner, c
uid = (uid_t) atoi(owner + 1);
} else if (pw == NULL || strcmp(owner, pw->pw_name) != 0) {
if ((pw = getpwnam(owner)) == NULL) {
-   if (mode != -1 && IS_ON(mode, S_ISUID)) {
+   if (IS_ON(mode, S_ISUID)) {
message(MT_NOTICE,
  "%s: unknown login name \"%s\", clearing setuid",
target, owner);
@@ -213,13 +210,10 @@ fchog(int fd, char *file, char *owner, c
} else {/* not root, setuid only if user==owner */
struct passwd *lupw;
 
-   if (mode != -1) {
-   if (IS_ON(mode, S_ISUID) && 
-   strcmp(locuser, owner) != 0)
-   mode &= ~S_ISUID;
-   if (mode)
-   mode &= ~S_ISVTX; /* and strip sticky too */
-   }
+   if (IS_ON(mode, S_ISUID) && strcmp(locuser, owner) != 0)
+   mode &= ~S_ISUID;
+   if (mode)
+   mode &= ~S_ISVTX; /* and strip sticky too */
 
if ((lupw = getpwnam(locuser)) != NULL)
primegid = lupw->pw_gid;
@@ -230,7 +224,7 @@ fchog(int fd, char *file, char *owner, c
if ((*group == ':' && 
 (getgrgid(gid = atoi(group + 1)) == NULL))
|| ((gr = (struct group *)getgrnam(group)) == NULL)) {
-   

Re: rdistd: quiet compiler warnings

[Todd C. Miller]

On Wed, 30 Mar 2016 14:29:05 -0600, Theo de Raadt wrote:

> > /usr/src/usr.bin/rdistd/server.c:845: warning: zero-length printf format st
> ring
> 
> defs.h:void error(const char *, ...) __attribute__((format (printf, 1, 2)));
> 
> That seems to be the source of this warning.  That function is not
> printf-like, in that it produces an implicit newline...
> 
> Shrug, it is contained within this program.  Whatever works.

It is no worse than warn(3) and err(3) in that respect.  All we
really want from the printf format attribute is to help avoid printf
format bugs.

 - todd

move "privileged port" check out of in(6)_pcbaddrisavail()

[Vincent Gross]

Hello,

This diff moves the "are we binding to a privileged port while not being root ?"
check from in(6)_pcbaddrisavail() to in_pcbbind().

This way we have a cleaner separation between "is the resource available ?"
and "am I allowed to access the resource ?" (which may or may not get its own
function later).

Also, it unbreaks naddy@'s iked setup (ikev2:sendmsg([::]:500) =>
in6_selectsrc() != in6p->inp_laddr6 => in6_pcbaddrisavail() => EPERM).

Ok ?

Index: sys/netinet/in_pcb.c
===
RCS file: /cvs/src/sys/netinet/in_pcb.c,v
retrieving revision 1.198
diff -u -p -r1.198 in_pcb.c
--- sys/netinet/in_pcb.c26 Mar 2016 21:56:04 -  1.198
+++ sys/netinet/in_pcb.c30 Mar 2016 20:33:00 -
@@ -341,9 +341,14 @@ in_pcbbind(struct inpcb *inp, struct mbu
}
}
 
-   if (lport == 0)
+   if (lport == 0) {
if ((error = in_pcbpickport(, wild, inp, p)))
return (error);
+   } else {
+   if (ntohs(lport) < IPPORT_RESERVED &&
+   (error = suser(p, 0)))
+   return (EACCES);
+   }
inp->inp_lport = lport;
in_pcbrehash(inp);
return (0);
@@ -357,7 +362,6 @@ in_pcbaddrisavail(struct inpcb *inp, str
struct inpcbtable *table = inp->inp_table;
u_int16_t lport = sin->sin_port;
int reuseport = (so->so_options & SO_REUSEPORT);
-   int error;
 
if (IN_MULTICAST(sin->sin_addr.s_addr)) {
/*
@@ -398,9 +402,6 @@ in_pcbaddrisavail(struct inpcb *inp, str
struct inpcb *t;
 
/* GROSS */
-   if (ntohs(lport) < IPPORT_RESERVED &&
-   (error = suser(p, 0)))
-   return (EACCES);
if (so->so_euid) {
t = in_pcblookup(table, _addr, 0,
>sin_addr, lport, INPLOOKUP_WILDCARD,
Index: sys/netinet6/in6_pcb.c
===
RCS file: /cvs/src/sys/netinet6/in6_pcb.c,v
retrieving revision 1.90
diff -u -p -r1.90 in6_pcb.c
--- sys/netinet6/in6_pcb.c  30 Mar 2016 13:02:22 -  1.90
+++ sys/netinet6/in6_pcb.c  30 Mar 2016 20:33:01 -
@@ -158,7 +158,6 @@ in6_pcbaddrisavail(struct inpcb *inp, st
struct inpcbtable *table = inp->inp_table;
u_short lport = sin6->sin6_port;
int reuseport = (so->so_options & SO_REUSEPORT);
-   int error;
 
wild |= INPLOOKUP_IPV6;
/* KAME hack: embed scopeid */
@@ -226,8 +225,6 @@ in6_pcbaddrisavail(struct inpcb *inp, st
 * finding a process for a socket instead of using
 * curproc?  (Marked with BSD's {in,}famous XXX ?
 */
-   if (ntohs(lport) < IPPORT_RESERVED && (error = suser(p, 0)))
-   return error;
if (so->so_euid) {
t = in_pcblookup(table,
(struct in_addr *)_addr, 0,

Re: increase v_specbitmap size (allow more cloned devices)

[Todd C. Miller]

On Wed, 30 Mar 2016 12:32:40 -0700, Philip Guenther wrote:

> That structure is used for every (open?) device, no?  Is there an
> estimate of memory usage increase?  Maybe the bitmap should be
> separately allocated for the cloning device, as there's only a few of
> those ever.

That's a good point.  The old ci_bitmap[] was the same size as a
pointer on 64-bit platforms so it was basically free.  Now that it
is much larger it probably makes sense to only allocate it for the
cloning device.

 - todd

Re: rdistd: quiet compiler warnings

[Theo de Raadt]

> /usr/src/usr.bin/rdistd/server.c:845: warning: zero-length printf format 
> string

defs.h:void error(const char *, ...) __attribute__((format (printf, 1, 2)));

That seems to be the source of this warning.  That function is not
printf-like, in that it produces an implicit newline...

Shrug, it is contained within this program.  Whatever works.

Re: increase v_specbitmap size (allow more cloned devices)

[Theo de Raadt]

>On Wed, Mar 30, 2016 at 7:31 AM, Martin Natano  wrote:
>> I'm currently working on a diff to make bpf a cloning device. Therefore
>> it is necessary to increase the number of clones possible of a cloning
>> device, as there are users with a need for more than 64 open bpf devices
>> at the same time. mikeb@ pointed me to this thread:
>> https://marc.info/?l=openbsd-tech=135410367503770
>>
>> Objections/Ok?
>
>That structure is used for every (open?) device, no?  Is there an
>estimate of memory usage increase?  Maybe the bitmap should be
>separately allocated for the cloning device, as there's only a few of
>those ever.

Yes this seems like the right place to use a pointer, it will also
shrink the objectsize for the default case.

rdistd: quiet compiler warnings

[Todd C. Miller]

This fixed the following warnings:

/usr/src/usr.bin/rdistd/server.c:845: warning: zero-length printf format string
/usr/src/usr.bin/rdistd/server.c:1150: warning: zero-length printf format string

The error() function already supports passing a NULL format string.
This diff allows message() to handle a NULL format string and changes
the instances of error("") to error(NULL) and message(..., "") to
message(..., NULL).

 - todd

Index: usr.bin/rdist/message.c
===
RCS file: /cvs/src/usr.bin/rdist/message.c,v
retrieving revision 1.27
diff -u -p -r1.27 message.c
--- usr.bin/rdist/message.c 20 Jan 2015 09:00:16 -  1.27
+++ usr.bin/rdist/message.c 30 Mar 2016 20:16:47 -
@@ -591,11 +591,13 @@ message(int lvl, const char *fmt, ...)
static char buf[MSGBUFSIZ];
va_list args;
 
-   va_start(args, fmt);
-   (void) vsnprintf(buf, sizeof(buf), fmt, args);
-   va_end(args);
+   if (fmt != NULL) {
+   va_start(args, fmt);
+   (void) vsnprintf(buf, sizeof(buf), fmt, args);
+   va_end(args);
+   }
 
-   _message(lvl, buf);
+   _message(lvl, fmt ? buf : NULL);
 }
 
 /*
Index: usr.bin/rdistd/server.c
===
RCS file: /cvs/src/usr.bin/rdistd/server.c,v
retrieving revision 1.41
diff -u -p -r1.41 server.c
--- usr.bin/rdistd/server.c 30 Mar 2016 17:03:06 -  1.41
+++ usr.bin/rdistd/server.c 30 Mar 2016 20:16:47 -
@@ -839,7 +839,7 @@ recvfile(char *new, opt_t opts, int mode
 * need to indicate to the master that
 * the file was not updated.
 */
-   error("");
+   error(NULL);
return;
}
debugmsg(DM_MISC, "Files are different '%s' '%s'.",
@@ -1144,7 +1144,7 @@ recvlink(char *new, opt_t opts, int mode
 
if (IS_ON(opts, DO_VERIFY) || uptodate) {
if (uptodate)
-   message(MT_REMOTE|MT_INFO, "");
+   message(MT_REMOTE|MT_INFO, NULL);
else
message(MT_REMOTE|MT_INFO, "%s: need to update",
target);

Re: increase v_specbitmap size (allow more cloned devices)

[Philip Guenther]

On Wed, Mar 30, 2016 at 7:31 AM, Martin Natano  wrote:
> I'm currently working on a diff to make bpf a cloning device. Therefore
> it is necessary to increase the number of clones possible of a cloning
> device, as there are users with a need for more than 64 open bpf devices
> at the same time. mikeb@ pointed me to this thread:
> https://marc.info/?l=openbsd-tech=135410367503770
>
> Objections/Ok?

That structure is used for every (open?) device, no?  Is there an
estimate of memory usage increase?  Maybe the bitmap should be
separately allocated for the cloning device, as there's only a few of
those ever.


Philip Guenther

Re: [PATCH] Proposal to remove -f for arp(8) and ndp(8)

[Jeremie Courreges-Anglas]

Dimitris Papastamos  writes:

> On Wed, Mar 30, 2016 at 02:49:06PM +0200, Mike Belopuhov wrote:
>> Good day, Dimitris.
>> 
>> Long time ago in a galaxy far far away I've been using this
>> alongside the -F option that I've added.  While managed
>> switches are becoming cheaper, I don't see a reason for a
>> working feature to go away, especially since there has been
>> zero rationale provided apart from "ndp -f" doesn't work.
>> Well, then how about fixing ndp?  Half of ndp(8) features
>> don't work correctly (hello rdomains), but it's not a valid
>> reason to go ahead and rip useful bits out.
>
> Thanks for the feedback.  I will dig out the first patch I sent
> which basically fixed ndp(8) instead of removing the feature.

oops, I missed your mail and sent another diff.  Will take a look at it
soon.

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: multi-pool malloc wip diff

[Norman Golisz]

On Mon Mar 28 2016 11:27, Otto Moerbeek wrote:
> Second diff. Only one person (Stefan Kempf, thanks!) gave feedback...

Sorry, running with this patch since a week, but missed to give
feedback.

As others already reported, no regressions here on amd64 also.

Re: [PATCH] Proposal to remove -f for arp(8) and ndp(8)

[Jeremie Courreges-Anglas]

Mike Belopuhov  writes:

> Good day, Dimitris.
>
> Long time ago in a galaxy far far away I've been using this
> alongside the -F option that I've added.  While managed
> switches are becoming cheaper, I don't see a reason for a
> working feature to go away, especially since there has been
> zero rationale provided apart from "ndp -f" doesn't work.
> Well, then how about fixing ndp?
[...]

Diff below, seems to work fine.

Index: ndp.c
===
RCS file: /cvs/src/usr.sbin/ndp/ndp.c,v
retrieving revision 1.69
diff -u -p -p -u -r1.69 ndp.c
--- ndp.c   26 Jan 2016 18:26:19 -  1.69
+++ ndp.c   30 Mar 2016 11:49:50 -
@@ -241,6 +241,11 @@ main(int argc, char *argv[])
}
delete(arg);
break;
+   case 'f':
+   if (argc != 0)
+   usage();
+   file(arg);
+   break;
case 'p':
if (argc != 0) {
usage();


-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: [patch] Fix carp(4) with balancing ip / ip-stealth

[Florian Riehm]

On 03/01/16 23:03, Martin Pieuchot wrote:
> On 18/02/16(Thu) 16:46, Florian Riehm wrote:
>> On 02/16/16 11:23, Martin Pieuchot wrote:
>>> On 12/02/16(Fri) 16:33, Florian Riehm wrote:
 Hi Tech,

 I have noticed that CARP IP-Balancing is broken, so I am testing and
 fixing it.

 The first problem came in with this commit:
 http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/net/if_ethersubr.c.diff?r1=1.176=1.177
 It enforces that outgoing packets use the src mac of the carp interface 
 instead
 the mac of its carpdev. That's a good idea for failover carp but it breaks
 balancing ip(-stealth), because the same source MAC is seen by multiple
 switchports now. That leads to continues MAC flapping at switches.
 My attached patch fixes the problem. Thanks Martin's refactoring, we can
 enforce the right MAC on a central place in carp_start() now.
>>>
>>> Can't you completely get rid of the else chunk then?  It looks like a
>>> bad refactoring.
>>
>> I was also thinking about deleting the else chunk. At the end I decided to 
>> keep
>> it, because I wasn't sure about its purpose.
>> My intention was:
>> Fix the more or less obviously broken balancing case and don't change the
>> behavior of other CARP modes.
>> I will remove the else chunk and do some more testing to determine if it's
>> really unnecessary.
>>
>>> I'm also wondering can't we use "sc_realmac" here?
>>
>> I have not know "sc_realmac" until now. As far as I understand the intention 
>> of
>> the sc_realmac flag is, to indicate that the user has overwritten the mac
>> address of the carp interface with the mac of its parent. I have never had a
>> use case to do this. Without overwriting the mac by hand sc_realmac is 0 for
>> carp with balancing ip/ip-stealth AND carp without balancing.
>>
>> I don't see how sc_realmac could help us by this problem?!
>> Can you give me a hint please?
> 
> It was an open question, if that does not make any sense, then don't do
> it ;)
> 
 A second problem was introduced two weeks ago. Since we always check the
 destination MAC address of received unicast packets, not only when in
 promiscuous mode, carp balancing ip is always broken, not only when in
 promiscuous mode. The new check collides with "Clear mcast if received on a
 carp IP balanced address." in carp_input().
>>>
>>> Could you describe the problem?  Which Destination MAC the packet
>>> contains and why it doesn't match the interface?  I still don't grok why
>>> a carp(4) interface in balancing mode cannot have the right MAC
>>> configured...  Do you know?  But if what you want is the parent's MAC,
>>> then I'd rather go with the symmetric of your other change:  modify
>>> carp_input() to overwrite ``ether_dhost''.
>>
>> If using carp balancing, incoming packets contain the destination mac address
>> of the carp interface, let's say: 01:00:5e:00:01:01.
>> The MCAST-Bit is set here!
>>
>> carp_input() removes the MCAST-Bit:
>>  /*
>>   * Clear mcast if received on a carp IP balanced address.
>>   */
>>  if (sc->sc_balancing == CARP_BAL_IP &&
>>  ETHER_IS_MULTICAST(eh->ether_dhost))
>>  *(eh->ether_dhost) &= ~0x01;
>>
>> Then we run into ether_input() and 
>> memcmp(ac->ac_enaddr, eh->ether_dhost, ETHER_ADDR_LEN)
>> evaluates to false because 
>> ac->ac_enaddr (01:00:5e:00:01:01) != eh->ether_dhost (00:00:5e:00:01:01)
> 
> Taking a step back why do you want to clear the MULTICAST bit?
> 
> If it is just to differentiate between advertisement and normal traffic
> couldn't we use a mbuf_tag(9) for CARP advertisement instead?  The tag
> could be set in carp_input() and checked in carp_lsdrop() in the input
> path.  It could also be used in the output path to get rid of the
> horrible ``cur_vhe'' hack.
> 
Thanks for your comments. I took a look to the big picture:
The idea behind carp balancing is that clients send all their packets to layer 2
multicast addresses. Switches flood those packets to all ports and every
CARP-System receives them. Only one system processes the packet, the other
systems drop it. The processing system handles the packet like a normal unicast
packet. The packet distribution described above is the only reason why the
multicast bit is required. We don't want further multicast handling.

What happens if we don't remove the MCAST bit inside carp_input()?
In ether_input() packets with MCAST bit get the mbuf flag M_MCAST. From now the
kernel assumes to deal with real multicast packets. Upper layers will do many
things we don't want, e.g. tcp_input() and icmp_input_if() would drop the
packet.
 
What can we do?
1.) Introduce a new mbuf flag to mark the special carp MCAST packets and handle
them properly.
=> Bad idea, because we have no unused bits inside m_flags. It would also spread
the CARP hacks to more places - that's not what we want.

2.) Remove the multicast bit of the carp interface's mac address to make the
check 

Re: list manual upgrade for single processor in upgrade59.html

[Theo Buehler]

> This adds manual upgrade instructions for bsd.sp kernels similar to what
> upgrade58 did.
> 
> Don't want to miss the nice copy & paste for all kind of machines I support.

good point. I added something similar to your diff back. Will be live
soon. Untested, so please double check.

list manual upgrade for single processor in upgrade59.html

[Kapetanakis Giannis]

Hi,

This adds manual upgrade instructions for bsd.sp kernels similar to what 
upgrade58 did.


Don't want to miss the nice copy & paste for all kind of machines I support.

regards,

Giannis

Index: upgrade59.html
===
RCS file: /cvs/www/faq/upgrade59.html,v
retrieving revision 1.19
diff -u -p -r1.19 upgrade59.html
--- upgrade59.html  29 Mar 2016 11:17:47 -  1.19
+++ upgrade59.html  30 Mar 2016 14:31:55 -
@@ -306,12 +306,25 @@ access to the system console.
   Install new kernels.
 The extra steps for copying over the primary kernel are done
 to ensure that there is always a valid kernel on the disk.
-
-cd /usr/rel# where you put the release files
-ln -f /bsd /obsd && cp bsd.mp /nbsd && mv /nbsd /bsd
-cp bsd.rd /
-cp bsd /bsd.sp
-
+
+  
+  If you are using a multiprocessor kernel:
+
+cd /usr/rel# where you put the release files
+ln -f /bsd /obsd && cp bsd.mp /nbsd && mv /nbsd /bsd
+cp bsd.rd /
+cp bsd /bsd.sp
+
+
+  If you are using a single processor kernel:
+
+cd /usr/rel# where you put the release files
+ln -f /bsd /obsd && cp bsd /nbsd && mv /nbsd /bsd
+cp bsd.rd bsd.mp /
+
+  (note: you will get a harmless error message if your platform
+  doesn't have a bsd.mp)
+
 
   

   Install new userland.

increase v_specbitmap size (allow more cloned devices)

[Martin Natano]

I'm currently working on a diff to make bpf a cloning device. Therefore
it is necessary to increase the number of clones possible of a cloning
device, as there are users with a need for more than 64 open bpf devices
at the same time. mikeb@ pointed me to this thread:
https://marc.info/?l=openbsd-tech=135410367503770

Objections/Ok?

natano


Index: sys/specdev.h
===
RCS file: /cvs/src/sys/sys/specdev.h,v
retrieving revision 1.34
diff -u -p -r1.34 specdev.h
--- sys/specdev.h   2 Nov 2013 00:16:31 -   1.34
+++ sys/specdev.h   30 Mar 2016 13:48:00 -
@@ -46,7 +46,7 @@ struct specinfo {
daddr_t si_lastr;
union {
struct vnode *ci_parent; /* pointer back to parent device */
-   u_int8_t ci_bitmap[8]; /* bitmap of devices cloned off us */
+   u_int8_t ci_bitmap[128]; /* bitmap of devices cloned off us */
} si_ci;
 };
 

Re: [PATCH] Proposal to remove -f for arp(8) and ndp(8)

[Dimitris Papastamos]

On Wed, Mar 30, 2016 at 02:49:06PM +0200, Mike Belopuhov wrote:
> Good day, Dimitris.
> 
> Long time ago in a galaxy far far away I've been using this
> alongside the -F option that I've added.  While managed
> switches are becoming cheaper, I don't see a reason for a
> working feature to go away, especially since there has been
> zero rationale provided apart from "ndp -f" doesn't work.
> Well, then how about fixing ndp?  Half of ndp(8) features
> don't work correctly (hello rdomains), but it's not a valid
> reason to go ahead and rip useful bits out.

Thanks for the feedback.  I will dig out the first patch I sent
which basically fixed ndp(8) instead of removing the feature.

Re: [PATCH] Proposal to remove -f for arp(8) and ndp(8)

[Mike Belopuhov]

Good day, Dimitris.

Long time ago in a galaxy far far away I've been using this
alongside the -F option that I've added.  While managed
switches are becoming cheaper, I don't see a reason for a
working feature to go away, especially since there has been
zero rationale provided apart from "ndp -f" doesn't work.
Well, then how about fixing ndp?  Half of ndp(8) features
don't work correctly (hello rdomains), but it's not a valid
reason to go ahead and rip useful bits out.

Regards,
Mike

On 30 March 2016 at 12:23, Dimitris Papastamos  wrote:
> Hi everyone,
>
> I totally forgot about this patch.  At the time it couldn't go in
> because the tree was locked.  Does it make sense?  If so I will check
> whether it applies on -current and resubmit.
>
> On Fri, Jul 31, 2015 at 01:55:07PM +0100, Dimitris Papastamos wrote:
>> Hi everyone,
>>
>> This is a patch that removes -f for arp(8) and ndp(8).  As it stands
>> currently, ndp(8) -f is not hooked in the code so no one is probably
>> using that.
>>
>> arp(8) -f is currently functional but I am not sure how useful.  If you
>> are using this option, please reply to this thread.
>>
>> Below you will find a patch that removes -f for both of these tools.
>>
>> Cheers,
>> Dimitris
>>

Re: [PATCH] Proposal to remove -f for arp(8) and ndp(8)

[Jeremie Courreges-Anglas]

Dimitris Papastamos  writes:

> Hi everyone,

Hi,

> I totally forgot about this patch.  At the time it couldn't go in
> because the tree was locked.  Does it make sense?  If so I will check
> whether it applies on -current and resubmit.

I don't understand the rationale.  Using -f sounds nicer than forcing
possible users to write yet another shell script.  The code is already
there, it is simple and short, so why remove it?

> On Fri, Jul 31, 2015 at 01:55:07PM +0100, Dimitris Papastamos wrote:
>> Hi everyone,
>> 
>> This is a patch that removes -f for arp(8) and ndp(8).  As it stands
>> currently, ndp(8) -f is not hooked in the code so no one is probably
>> using that.

Well maybe it can be fixed instead.

>> arp(8) -f is currently functional but I am not sure how useful.  If you
>> are using this option, please reply to this thread.
>> 
>> Below you will find a patch that removes -f for both of these tools.
>> 
>> Cheers,
>> Dimitris
>> 
>> ===
>> RCS file: /cvs/src/usr.sbin/arp/arp.8,v
>> retrieving revision 1.36
>> diff -u -p -r1.36 arp.8
>> --- usr.sbin/arp/arp.8   27 Jul 2015 17:28:39 -  1.36
>> +++ usr.sbin/arp/arp.8   31 Jul 2015 12:51:29 -
>> @@ -43,7 +43,6 @@
>>  .Ar hostname
>>  .Nm arp
>>  .Op Fl F
>> -.Op Fl f Ar file
>>  .Op Fl V Ar rdomain
>>  .Fl s Ar hostname ether_addr
>>  .Op Cm temp | permanent
>> @@ -123,37 +122,6 @@ Force existing entries for the given hos
>>  and
>>  .Fl s
>>  options).
>> -.It Fl f Ar file
>> -Process entries from
>> -.Ar file
>> -to be set in the ARP tables.
>> -Any entries in the file that already exist for a given host
>> -will not be overwritten unless
>> -.Fl F
>> -is given.
>> -Entries in the file should be of the form:
>> -.Bd -filled -offset indent
>> -.Ar hostname ether_addr
>> -.Op Cm temp | permanent
>> -.Op Cm pub
>> -.Ed
>> -.Pp
>> -The entry will be static (will not time out) unless the word
>> -.Cm temp
>> -is given in the command.
>> -A static ARP entry can be overwritten by network traffic, unless the word
>> -.Cm permanent
>> -is given.
>> -If the word
>> -.Cm pub
>> -is given, the entry will be
>> -.Dq published ;
>> -that is, this system will act as an ARP server,
>> -responding to requests for
>> -.Ar hostname
>> -even though the host address is not its own.
>> -This behavior has traditionally been called
>> -.Em proxy ARP .
>>  .It Fl n
>>  Show network addresses as numbers (normally
>>  .Nm
>> Index: usr.sbin/arp/arp.c
>> ===
>> RCS file: /cvs/src/usr.sbin/arp/arp.c,v
>> retrieving revision 1.64
>> diff -u -p -r1.64 arp.c
>> --- usr.sbin/arp/arp.c   3 Jun 2015 08:10:53 -   1.64
>> +++ usr.sbin/arp/arp.c   31 Jul 2015 12:51:29 -
>> @@ -71,7 +71,6 @@ void nuke_entry(struct sockaddr_dl *sdl,
>>  struct sockaddr_inarp *sin, struct rt_msghdr *rtm);
>>  static char *ether_str(struct sockaddr_dl *);
>>  int wake(const char *ether_addr, const char *iface);
>> -int file(char *);
>>  int get(const char *);
>>  int getinetaddr(const char *, struct in_addr *);
>>  void getsocket(void);
>> @@ -97,9 +96,8 @@ extern int h_errno;
>>  /* which function we're supposed to do */
>>  #define F_GET   1
>>  #define F_SET   2
>> -#define F_FILESET   3
>> -#define F_DELETE4
>> -#define F_WAKE  5
>> +#define F_DELETE3
>> +#define F_WAKE  4
>>  
>>  int
>>  main(int argc, char *argv[])
>> @@ -131,11 +129,6 @@ main(int argc, char *argv[])
>>  case 'F':
>>  replace = 1;
>>  break;
>> -case 'f':
>> -if (func)
>> -usage();
>> -func = F_FILESET;
>> -break;
>>  case 'V':
>>  rdomain = strtonum(optarg, 0, RT_TABLEID_MAX, );
>>  if (errstr != NULL) {
>> @@ -184,11 +177,6 @@ main(int argc, char *argv[])
>>  else
>>  usage();
>>  break;
>> -case F_FILESET:
>> -if (argc != 1)
>> -usage();
>> -rtn = file(argv[0]);
>> -break;
>>  case F_WAKE:
>>  if (aflag || nflag || replace || rdomain > 0)
>>  usage();
>> @@ -203,41 +191,6 @@ main(int argc, char *argv[])
>>  return (rtn);
>>  }
>>  
>> -/*
>> - * Process a file to set standard arp entries
>> - */
>> -int
>> -file(char *name)
>> -{
>> -char line[100], arg[5][50], *args[5];
>> -int  i, retval;
>> -FILE*fp;
>> -
>> -if ((fp = fopen(name, "r")) == NULL)
>> -err(1, "cannot open %s", name);
>> -args[0] = [0][0];
>> -args[1] = [1][0];
>> -args[2] = [2][0];
>> -args[3] = [3][0];
>> -args[4] = [4][0];
>> -retval = 0;
>> -while (fgets(line, sizeof(line), fp) != NULL) {
>> -i = 

Re: [PATCH] Proposal to remove -f for arp(8) and ndp(8)

[Dimitris Papastamos]

Hi everyone,

I totally forgot about this patch.  At the time it couldn't go in
because the tree was locked.  Does it make sense?  If so I will check
whether it applies on -current and resubmit.

On Fri, Jul 31, 2015 at 01:55:07PM +0100, Dimitris Papastamos wrote:
> Hi everyone,
> 
> This is a patch that removes -f for arp(8) and ndp(8).  As it stands
> currently, ndp(8) -f is not hooked in the code so no one is probably
> using that.
> 
> arp(8) -f is currently functional but I am not sure how useful.  If you
> are using this option, please reply to this thread.
> 
> Below you will find a patch that removes -f for both of these tools.
> 
> Cheers,
> Dimitris
> 
> ===
> RCS file: /cvs/src/usr.sbin/arp/arp.8,v
> retrieving revision 1.36
> diff -u -p -r1.36 arp.8
> --- usr.sbin/arp/arp.827 Jul 2015 17:28:39 -  1.36
> +++ usr.sbin/arp/arp.831 Jul 2015 12:51:29 -
> @@ -43,7 +43,6 @@
>  .Ar hostname
>  .Nm arp
>  .Op Fl F
> -.Op Fl f Ar file
>  .Op Fl V Ar rdomain
>  .Fl s Ar hostname ether_addr
>  .Op Cm temp | permanent
> @@ -123,37 +122,6 @@ Force existing entries for the given hos
>  and
>  .Fl s
>  options).
> -.It Fl f Ar file
> -Process entries from
> -.Ar file
> -to be set in the ARP tables.
> -Any entries in the file that already exist for a given host
> -will not be overwritten unless
> -.Fl F
> -is given.
> -Entries in the file should be of the form:
> -.Bd -filled -offset indent
> -.Ar hostname ether_addr
> -.Op Cm temp | permanent
> -.Op Cm pub
> -.Ed
> -.Pp
> -The entry will be static (will not time out) unless the word
> -.Cm temp
> -is given in the command.
> -A static ARP entry can be overwritten by network traffic, unless the word
> -.Cm permanent
> -is given.
> -If the word
> -.Cm pub
> -is given, the entry will be
> -.Dq published ;
> -that is, this system will act as an ARP server,
> -responding to requests for
> -.Ar hostname
> -even though the host address is not its own.
> -This behavior has traditionally been called
> -.Em proxy ARP .
>  .It Fl n
>  Show network addresses as numbers (normally
>  .Nm
> Index: usr.sbin/arp/arp.c
> ===
> RCS file: /cvs/src/usr.sbin/arp/arp.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 arp.c
> --- usr.sbin/arp/arp.c3 Jun 2015 08:10:53 -   1.64
> +++ usr.sbin/arp/arp.c31 Jul 2015 12:51:29 -
> @@ -71,7 +71,6 @@ void nuke_entry(struct sockaddr_dl *sdl,
>   struct sockaddr_inarp *sin, struct rt_msghdr *rtm);
>  static char *ether_str(struct sockaddr_dl *);
>  int wake(const char *ether_addr, const char *iface);
> -int file(char *);
>  int get(const char *);
>  int getinetaddr(const char *, struct in_addr *);
>  void getsocket(void);
> @@ -97,9 +96,8 @@ extern int h_errno;
>  /* which function we're supposed to do */
>  #define F_GET1
>  #define F_SET2
> -#define F_FILESET3
> -#define F_DELETE 4
> -#define F_WAKE   5
> +#define F_DELETE 3
> +#define F_WAKE   4
>  
>  int
>  main(int argc, char *argv[])
> @@ -131,11 +129,6 @@ main(int argc, char *argv[])
>   case 'F':
>   replace = 1;
>   break;
> - case 'f':
> - if (func)
> - usage();
> - func = F_FILESET;
> - break;
>   case 'V':
>   rdomain = strtonum(optarg, 0, RT_TABLEID_MAX, );
>   if (errstr != NULL) {
> @@ -184,11 +177,6 @@ main(int argc, char *argv[])
>   else
>   usage();
>   break;
> - case F_FILESET:
> - if (argc != 1)
> - usage();
> - rtn = file(argv[0]);
> - break;
>   case F_WAKE:
>   if (aflag || nflag || replace || rdomain > 0)
>   usage();
> @@ -203,41 +191,6 @@ main(int argc, char *argv[])
>   return (rtn);
>  }
>  
> -/*
> - * Process a file to set standard arp entries
> - */
> -int
> -file(char *name)
> -{
> - char line[100], arg[5][50], *args[5];
> - int  i, retval;
> - FILE*fp;
> -
> - if ((fp = fopen(name, "r")) == NULL)
> - err(1, "cannot open %s", name);
> - args[0] = [0][0];
> - args[1] = [1][0];
> - args[2] = [2][0];
> - args[3] = [3][0];
> - args[4] = [4][0];
> - retval = 0;
> - while (fgets(line, sizeof(line), fp) != NULL) {
> - i = sscanf(line, "%49s %49s %49s %49s %49s", arg[0], arg[1],
> - arg[2], arg[3], arg[4]);
> - if (i < 2) {
> - warnx("bad line: %s", line);
> - retval = 1;
> - continue;
> - }
> - if (replace)
> - delete(arg[0], NULL);
> - if (set(i, args))

Re: knote activate splhigh

[Martin Pieuchot]

On 29/03/16(Tue) 22:36, Alexander Bluhm wrote:
> Hi,
>
> from a customer's system I got this panic:
>
> kernel diagnostic assertion "(kn->kn_status & KN_QUEUED) == 0" failed: file 
> "..
> /../../../kern/kern_event.c", line 1071
>
> panic() at panic+0xfe
> __assert() at __assert+0x25
> knote_enqueue() at knote_enqueue+0x8c
> knote() at knote+0x47
> selwakeup() at selwakeup+0x1b
> logwakeup() at logwakeup+0x20
> log() at log+0xfc
> ...
> softclock() at softclock+0x315
> softintr_dispatch() at softintr_dispatch+0x7f
>
> When looking at the condition in KNOTE_ACTIVATE()
> if ((kn->kn_status & (KN_QUEUED | KN_DISABLED)) == 0)
> knote_enqueue(kn);
> and the assertion in knote_enqueue()
> KASSERT((kn->kn_status & KN_QUEUED) == 0);
> it is quite obvious that interrupts must be blocked between those.
>
> So put the splhigh() around KNOTE_ACTIVATE() and use a splassert()
> within knote_enqueue().  This is more or less where FreeBSD puts
> its KQ_LOCK().

Don't you want to raise the SPL around all the places where kn_status
is checked?

Is knote_dequeue() safe?  Even if it is do we want to put a splassert()
in there to keep it in sync with knote_enqueue()?

I'm also wondering, don't you want to move the content of
KNOTE_ACTIVATE() to the function of the same name and also put an
splassert() there?

Are the splhigh() around kn_status in kqueue_register() safe?

> @@ -964,10 +971,14 @@ void
>  knote(struct klist *list, long hint)
>  {
>   struct knote *kn, *kn0;
> + int s;
>
> - SLIST_FOREACH_SAFE(kn, list, kn_selnext, kn0)
> + SLIST_FOREACH_SAFE(kn, list, kn_selnext, kn0) {
> + s = splhigh();
>   if (kn->kn_fop->f_event(kn, hint))
>   KNOTE_ACTIVATE(kn);
> + splx(s);
> + }
>  }

Here I'd simply take splhigh() for the whole loop.

BTW why are we using splhigh() and not softclock()?

Re: spamd - DNS whitelist - with prototype

[Christopher Zimmermann]

I forgot to attach my prototype. Here it is.

On 2016-03-29 Bob Beck  wrote:
> No.  DNS based whitelisting does not belong in there. because it is
> slow and DOS'able
> 
> spamd is designed to be high speed low drag. If you want to do a DNS
> based whitelist, write a little co-thing that spits one
> into a file or into your nospamd table that then spamd *does not even
> see*.

That's kind of what my prototype implementation does.

> In short *spamd* is the wrong place to do this.  put your dns based
> whitelist in a table periodically

I put the ips into the table on demand since I cannot simply download a
dnswl. I already suspected relayd might be a better place to do this.
Or keep it in a separate program. Sadly I cannot reinject the diverted
SYN into pf.

> On Tue, Mar 29, 2016 at 1:11 PM, Christopher Zimmermann
>  wrote:
> > Hi,
> >
> > I want to use a DNS white list to skip greylisting delays for known
> > good addresses, which would pass the greylist anyway.
> > To do this with spamd and OpenSMTPd I wrote a prototype which
> > intercepts the initial SYN packet from any non-whitelisted ip. It
> > then queries DNS whitelists and on any positive reply it whitelists
> > the ip. The SYN packet is dropped. Any sane smtp server will very
> > shortly resend the SYN and get through to OpenSMTPd.
> > This program is only a proof-of-concept. I think the same
> > functionality could be integrated into spamd or as transparent
> > relay into relayd. Is this a sensible approach?
> >
> > Christopher
> >
> >
> > On 2016-03-15 Stuart Henderson  wrote:  
> >> On 2016/03/15 12:55, Craig Skinner wrote:  
> >> > Generally, everything has changed from file feeds to DNS.  
> >>
> >> Yep, because for the more actively maintained ones 1) new entries
> >> show up more quickly than any sane rsync interval, this is quite
> >> important for good blocking these days 2) DNS is less resource
> >> intensive and more easily distributed than rsync, and 3)
> >> importantly for the rbl providers, it gives additional input to
> >> them about new mail sources (if an rbl suddenly starts seeing
> >> queries from all over the world for a previously unseen address,
> >> it's probably worth investigation - I am sure this is why some of
> >> the commercial antispam operators provide free DNS-based lookups
> >> for smaller orgs).
> >>
> >> A more flexible approach would be to skip the PF table integration
> >> completely and do DNS lookups in spamd (or, uh, relayd, or
> >> something new) and based on that it could choose whether to
> >> tarpit, greylist or transparent-forward the connection to the real
> >> mail server. This would also give a way to use dnswl.org's
> >> whitelist to avoid greylisting for those hosts where it just
> >> doesn't work well (gmail, office365 etc).


#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 
#include 


#define DEBUG 0

#define DIVERT_PORT 25

#define NSTATES 10

struct dns_header {
uint16_tid;
uint16_tflags;
#define QR 0x8000
#define OPCODE_MASK 0x7800
#define OPCODE_SHIFT 11
#define AA 0x0400
#define TC 0x0200
#define RD 0x0100
#define RA 0x0080
#define AD 0x0020
#define CD 0x0010
#define RCODE_MASK 0x000f
#define RCODE_SHIFT 0
uint16_tqdcount;
uint16_tancount;
uint16_tnscount;
uint16_tarcount;
};

struct dns_record {
uint16_ttype;
uint16_tclass;
uint32_tttl;
uint16_tlength;
};

struct state {
union {
struct in_addr in4;
struct in6_addr in6;
uint8_t octets[sizeof(struct in6_addr)];
} addr;
struct timespec timeout;
int af;
uint16_t dnskey;
} states[NSTATES];

void send_query(struct state *state, const char *question);
void process_response();

enum color { white, grey };
void enlist(struct state *state, enum color color);

int dnssock, pfdev;

const char *const whitelists[] = {
"list.dnswl.org",
"swl.spamhaus.org"
};

int main(int argc, char *argv[])
{
int i, ret, timeout;
time_t t;
struct sockaddr_in sin4;
struct sockaddr_in6 sin6;
struct group *group;
struct passwd *passwd;
struct pollfd fds[3];

pfdev = open("/dev/pf", O_RDWR);
if (pfdev == -1) err(1, "open(\"/dev/pf\") failed");

ret = IPPROTO_DIVERT_INIT;
setsockopt(fds[1].fd, IPPROTO_IP, IP_DIVERTFL, , sizeof(ret));
setsockopt(fds[2].fd, IPPROTO_IPV6, IP_DIVERTFL, , sizeof(ret));

/* DNS */
if (res_init() == -1) err(1, "res_init");
assert(_res_ext.nsaddr_list[0].ss_family != 0);
fds[0].fd = dnssock = socket(_res_ext.nsaddr_list[0].ss_family,
   SOCK_DGRAM | SOCK_DNS, 0);
if (fds[0].fd == -1) err(1, "socket");

if (connect(fds[0].fd, (struct sockaddr *)&_res_ext.nsaddr_list[0],