Re: Why store pointers for some functions in malloc.c?
From: Otto Moerbeek
Date: Wed, 18 Oct 2023 07:28:47 +0200
> On Wed, Oct 18, 2023 at 09:23:49AM +0900, Masato Asou wrote:
>
>> Hello tech@ and otto,
>>
>> Why do only some calling functions store the pinttes in region_info as
>> below:
>>
>> static void *
>> malloc_bytes(struct dir_info *d, size_t size, void *f)
>> {
>>
>> found:
>> if (i == 0 && k == 0 && DO_STATS) {
>> struct region_info *r = find(d, bp->page);
>> STATS_SETF(r, f);
>> }
>>
>> I found following mail from otto:
>> https://marc.info/?l=openbsd-tech=168171382927798=2
>> > The null "f" values (call sites) are due to the sampling nature of
>> > small allocations. Recording all call sites of all potential leaks
>> > introduces too much overhead.
>>
>> Is this the answer to my question?
>> --
>> ASOU Masato
>
> Yes.
>
> The reason is that (in the existing code) there's only one pointer per
> region_info available to store callers. So for a chunk page (which has
> many small alocations) ony slot 0 gets recorded.
OK.
> But there's a diff I posted last week on tech@ that will change this
> so that all call sites are recorded (in a different location and only
> if D is used). It will also report more details when a write of a free
> chunk is detected. That diff could use some review/testing.
I'll checkt the your posted diff.
Thank your for your information!
--
ASOU Masato
Re: Why store pointers for some functions in malloc.c?
On Wed, Oct 18, 2023 at 09:23:49AM +0900, Masato Asou wrote:
> Hello tech@ and otto,
>
> Why do only some calling functions store the pinttes in region_info as
> below:
>
> static void *
> malloc_bytes(struct dir_info *d, size_t size, void *f)
> {
>
> found:
> if (i == 0 && k == 0 && DO_STATS) {
> struct region_info *r = find(d, bp->page);
> STATS_SETF(r, f);
> }
>
> I found following mail from otto:
> https://marc.info/?l=openbsd-tech=168171382927798=2
> > The null "f" values (call sites) are due to the sampling nature of
> > small allocations. Recording all call sites of all potential leaks
> > introduces too much overhead.
>
> Is this the answer to my question?
> --
> ASOU Masato
Yes.
The reason is that (in the existing code) there's only one pointer per
region_info available to store callers. So for a chunk page (which has
many small alocations) ony slot 0 gets recorded.
But there's a diff I posted last week on tech@ that will change this
so that all call sites are recorded (in a different location and only
if D is used). It will also report more details when a write of a free
chunk is detected. That diff could use some review/testing.
-Otto
Re: pkg_add: No progress meter: failed termcap loop
On Tue, Oct 17, 2023 at 06:34:58PM -0700, Andrew Hewus Fresh wrote:
> On Tue, Oct 17, 2023 at 06:13:30PM -0700, Andrew Hewus Fresh wrote:
> > On Tue, Oct 17, 2023 at 05:10:44PM -0700, Greg Steuck wrote:
> > > I just got myself a fresh snapshot with libncurses.so.15.0:
> > > OpenBSD 7.4-current (GENERIC.MP) #1409: Tue Oct 17 17:08:49 MDT 2023
> > >
> > > I get some unhappiness now:
> > > # pkg_add -ui
> > > No progress meter: failed termcap loop
> > > quirks-6.161 signed on 2023-10-16T11:16:03Z
> > >
> > > Fallout from the ncurses upgrade?
And this is committed with an upstream PR created.
https://github.com/jonathanstowe/Term-Cap/pull/17
> Index: gnu/usr.bin/perl/cpan/Term-Cap/Cap.pm
> ===
> RCS file: /cvs/src/gnu/usr.bin/perl/cpan/Term-Cap/Cap.pm,v
> retrieving revision 1.2
> diff -u -p -r1.2 Cap.pm
> --- gnu/usr.bin/perl/cpan/Term-Cap/Cap.pm 5 Feb 2017 00:32:03 -
> 1.2
> +++ gnu/usr.bin/perl/cpan/Term-Cap/Cap.pm 18 Oct 2023 01:25:11 -
> @@ -280,7 +280,7 @@ sub Tgetent
>
> $first = 0;# first entry (keeps term name)
>
> -$max = 32; # max :tc=...:'s
> +$max = 64; # max :tc=...:'s
>
> if ($entry)
> {
>
Re: pkg_add: No progress meter: failed termcap loop
On Tue, Oct 17, 2023 at 06:34:58PM -0700, Andrew Hewus Fresh wrote:
> On Tue, Oct 17, 2023 at 06:13:30PM -0700, Andrew Hewus Fresh wrote:
> > On Tue, Oct 17, 2023 at 05:10:44PM -0700, Greg Steuck wrote:
> > > I just got myself a fresh snapshot with libncurses.so.15.0:
> > > OpenBSD 7.4-current (GENERIC.MP) #1409: Tue Oct 17 17:08:49 MDT 2023
> > >
> > > I get some unhappiness now:
> > > # pkg_add -ui
> > > No progress meter: failed termcap loop
> > > quirks-6.161 signed on 2023-10-16T11:16:03Z
> > >
> > > Fallout from the ncurses upgrade?
> >
> > Seems likely. I can reproduce with this code that's similar to
> > what OpenBSD::ProgressMeter::Term does:
> >
> > $ perl -MPOSIX -MTerm::Cap -e 'Term::Cap->Tgetent({ OSPEED =>
> > POSIX::Termios->new->getospeed })'
> > failed termcap loop at att610+cvis at -e line 1.
> >
> > or even just:
> >
> > $ perl -MTerm::Cap -e 'Term::Cap->Tgetent'
> >
> > failed termcap loop at att610+cvis at -e line 1.
> >
> > Wow though, the only hits google has for that "failed termcap loop" is
> > the file it is in so incredibly impressive.
>
> It appears 20 years ago nobody expected this complicated an
> /etc/termcap. This patch seems to fix the glitch.
>
> It has been 32 since at least it was moved back from SVN to CVS in 2004
> which is when git history stops.
> https://github.com/jonathanstowe/Term-Cap/blob/3a1193a8c5457d0ca77ea43d1be73712303a359c/Cap.pm#L254
Looking a bit harder, before that upstream for Term::Cap was perl core.
So that 32 limit dates back to 1995 and perl 5.001, I guess thinking 28
years into the future was enough.
https://github.com/Perl/perl5/commit/748a93069b3d16374a9859d1456065dd3ae11394#diff-0961d7f68bf68cb479f1fbd33f77f275bab83bfab124c64ab82b20ec5e9d6d87R77
> (I only needed to change this to 36, but how future proof is that?)
>
> Comments, OK?
>
>
> Index: gnu/usr.bin/perl/cpan/Term-Cap/Cap.pm
> ===
> RCS file: /cvs/src/gnu/usr.bin/perl/cpan/Term-Cap/Cap.pm,v
> retrieving revision 1.2
> diff -u -p -r1.2 Cap.pm
> --- gnu/usr.bin/perl/cpan/Term-Cap/Cap.pm 5 Feb 2017 00:32:03 -
> 1.2
> +++ gnu/usr.bin/perl/cpan/Term-Cap/Cap.pm 18 Oct 2023 01:25:11 -
> @@ -280,7 +280,7 @@ sub Tgetent
>
> $first = 0;# first entry (keeps term name)
>
> -$max = 32; # max :tc=...:'s
> +$max = 64; # max :tc=...:'s
>
> if ($entry)
> {
>
--
andrew
Hey, I think I see a barn up ahead.
-- The American Astronaut
mention U-Boot file and offset for Rockchip RK356x
ok? Index: distrib/notes/arm64/prep === RCS file: /cvs/src/distrib/notes/arm64/prep,v retrieving revision 1.18 diff -u -p -u -p -r1.18 prep --- distrib/notes/arm64/prep10 Apr 2023 12:57:15 - 1.18 +++ distrib/notes/arm64/prep18 Oct 2023 01:29:11 - @@ -132,3 +132,8 @@ Install on systems without a supported m of=/dev/sdXc seek=64 dd if=/usr/local/share/u-boot/board/u-boot.itb \ of=/dev/sdXc seek=16384 + + For systems based on Rockchip RK356x SoCs: + + dd if=/usr/local/share/u-boot/board/u-boot-rockchip.bin \ + of=/dev/sdXc seek=64
Re: pkg_add: No progress meter: failed termcap loop
On Tue, Oct 17, 2023 at 06:13:30PM -0700, Andrew Hewus Fresh wrote:
> On Tue, Oct 17, 2023 at 05:10:44PM -0700, Greg Steuck wrote:
> > I just got myself a fresh snapshot with libncurses.so.15.0:
> > OpenBSD 7.4-current (GENERIC.MP) #1409: Tue Oct 17 17:08:49 MDT 2023
> >
> > I get some unhappiness now:
> > # pkg_add -ui
> > No progress meter: failed termcap loop
> > quirks-6.161 signed on 2023-10-16T11:16:03Z
> >
> > Fallout from the ncurses upgrade?
>
> Seems likely. I can reproduce with this code that's similar to
> what OpenBSD::ProgressMeter::Term does:
>
> $ perl -MPOSIX -MTerm::Cap -e 'Term::Cap->Tgetent({ OSPEED =>
> POSIX::Termios->new->getospeed })'
> failed termcap loop at att610+cvis at -e line 1.
>
> or even just:
>
> $ perl -MTerm::Cap -e 'Term::Cap->Tgetent'
>
> failed termcap loop at att610+cvis at -e line 1.
>
> Wow though, the only hits google has for that "failed termcap loop" is
> the file it is in so incredibly impressive.
It appears 20 years ago nobody expected this complicated an
/etc/termcap. This patch seems to fix the glitch.
It has been 32 since at least it was moved back from SVN to CVS in 2004
which is when git history stops.
https://github.com/jonathanstowe/Term-Cap/blob/3a1193a8c5457d0ca77ea43d1be73712303a359c/Cap.pm#L254
(I only needed to change this to 36, but how future proof is that?)
Comments, OK?
Index: gnu/usr.bin/perl/cpan/Term-Cap/Cap.pm
===
RCS file: /cvs/src/gnu/usr.bin/perl/cpan/Term-Cap/Cap.pm,v
retrieving revision 1.2
diff -u -p -r1.2 Cap.pm
--- gnu/usr.bin/perl/cpan/Term-Cap/Cap.pm 5 Feb 2017 00:32:03 -
1.2
+++ gnu/usr.bin/perl/cpan/Term-Cap/Cap.pm 18 Oct 2023 01:25:11 -
@@ -280,7 +280,7 @@ sub Tgetent
$first = 0;# first entry (keeps term name)
-$max = 32; # max :tc=...:'s
+$max = 64; # max :tc=...:'s
if ($entry)
{
Re: pkg_add: No progress meter: failed termcap loop
On Tue, Oct 17, 2023 at 05:10:44PM -0700, Greg Steuck wrote:
> I just got myself a fresh snapshot with libncurses.so.15.0:
> OpenBSD 7.4-current (GENERIC.MP) #1409: Tue Oct 17 17:08:49 MDT 2023
>
> I get some unhappiness now:
> # pkg_add -ui
> No progress meter: failed termcap loop
> quirks-6.161 signed on 2023-10-16T11:16:03Z
>
> Fallout from the ncurses upgrade?
Seems likely. I can reproduce with this code that's similar to
what OpenBSD::ProgressMeter::Term does:
$ perl -MPOSIX -MTerm::Cap -e 'Term::Cap->Tgetent({ OSPEED =>
POSIX::Termios->new->getospeed })'
failed termcap loop at att610+cvis at -e line 1.
or even just:
$ perl -MTerm::Cap -e 'Term::Cap->Tgetent'
failed termcap loop at att610+cvis at -e line 1.
Wow though, the only hits google has for that "failed termcap loop" is
the file it is in so incredibly impressive.
l8rZ,
--
andrew
The programmer's national anthem is 'GH!!'.
Why store pointers for some functions in malloc.c?
Hello tech@ and otto,
Why do only some calling functions store the pinttes in region_info as
below:
static void *
malloc_bytes(struct dir_info *d, size_t size, void *f)
{
found:
if (i == 0 && k == 0 && DO_STATS) {
struct region_info *r = find(d, bp->page);
STATS_SETF(r, f);
}
I found following mail from otto:
https://marc.info/?l=openbsd-tech=168171382927798=2
> The null "f" values (call sites) are due to the sampling nature of
> small allocations. Recording all call sites of all potential leaks
> introduces too much overhead.
Is this the answer to my question?
--
ASOU Masato
Re: HAMMER2 filesystem for OpenBSD
Chris Narkiewicz writes: > On Tue, 2023-10-17 at 16:22 -0600, Theo de Raadt wrote: >> What will come of this discussion of opinions? > > If it's doable and the code quality is acceptable, I see 2 outcomes: > 1. I could start looking for sponsorship, as I feel like my level of > expertise is too limited to chew it myself > 2. perhaps with a right guidance I could try to bite it myself (not so > sure about it, but keen on finding out). > > I'm trying to contact the author atm to discuss his position and his > plans regarding the code so see if there is any alignment. > > Week ago, he added a disclaimer in the readme that he plans shift his > focus when FreeBSD write support is stabilized. If there is a limited > window of opportunity, I thought it's worth looking at it while there > is active interest. > > TBH, I don't know what would it take. The patch looks minimal, but I > suspect it's just tip of the iceberg and I lack the knowledge to > estimate how big the underwater part is. Especially the cost of > maintenance. Therefore, I decided to reach out to tech, kindly asking > about devs opinion or technical expertise on that matter. Pointing to a repository with a diff against an old tree is minimally interesting. If you report that you tested it and detail the outcome, the level of interest may rise to the point that somebody else might try. You can substantially increase the chances by forward-porting the change to -current and testing it. Thanks Greg
pkg_add: No progress meter: failed termcap loop
I just got myself a fresh snapshot with libncurses.so.15.0: OpenBSD 7.4-current (GENERIC.MP) #1409: Tue Oct 17 17:08:49 MDT 2023 I get some unhappiness now: # pkg_add -ui No progress meter: failed termcap loop quirks-6.161 signed on 2023-10-16T11:16:03Z Fallout from the ncurses upgrade? Thanks Greg
syslogd udp dropped counter
Hi,
Now that syslogd handles delayed DNS lookups, I think we should
count dropped packets to UDP loghosts. Although not every dropped
UDP packet can be detected, the message makes the admin aware that
there may be a blind spot during startup.
syslogd[26493]: dropped 8 messages to udp loghost "@udp://loghost"
Debug and log messages were improved, especially if UDP looging is
shut down permanently. Also do not print 'last message repeated'
if the message was dropped.
ok?
bluhm
Index: usr.sbin/syslogd/syslogd.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.278
diff -u -p -r1.278 syslogd.c
--- usr.sbin/syslogd/syslogd.c 12 Oct 2023 22:36:54 - 1.278
+++ usr.sbin/syslogd/syslogd.c 17 Oct 2023 23:30:05 -
@@ -241,6 +241,7 @@ int NoVerify = 0; /* do not verify TLS
const char *ClientCertfile = NULL;
const char *ClientKeyfile = NULL;
const char *ServerCAfile = NULL;
+intudpsend_dropped = 0;/* messages dropped due to UDP not ready */
inttcpbuf_dropped = 0; /* count messages dropped from TCP or TLS */
intfile_dropped = 0; /* messages dropped due to file system full */
intinit_dropped = 0; /* messages dropped during initialization */
@@ -1820,6 +1821,7 @@ logmsg(struct msg *msg, int flags, char
(f->f_type != F_PIPE && f->f_type != F_FORWUDP &&
f->f_type != F_FORWTCP && f->f_type != F_FORWTLS))) &&
(flags & MARK) == 0 && msglen == f->f_prevlen &&
+ f->f_dropped == 0 &&
!strcmp(msg->m_msg, f->f_prevline) &&
!strcmp(from, f->f_prevhost)) {
strlcpy(f->f_lasttime, msg->m_timestamp,
@@ -2005,14 +2007,14 @@ fprintlog(struct filed *f, int flags, ch
switch (f->f_type) {
case F_UNUSED:
- log_debug("%s", "");
+ log_debug("");
break;
case F_FORWUDP:
- log_debug(" %s", f->f_un.f_forw.f_loghost);
+ log_debugadd(" %s", f->f_un.f_forw.f_loghost);
if (f->f_un.f_forw.f_addr.ss_family == AF_UNSPEC) {
- log_warnx("not resolved \"%s\"",
- f->f_un.f_forw.f_loghost);
+ log_debug(" (dropped not resolved)");
+ f->f_dropped++;
break;
}
l = iov[0].iov_len + iov[1].iov_len + iov[2].iov_len +
@@ -2040,14 +2042,30 @@ fprintlog(struct filed *f, int flags, ch
case ENETUNREACH:
case ENOBUFS:
case EWOULDBLOCK:
+ log_debug(" (dropped send error)");
+ f->f_dropped++;
/* silently dropped */
break;
default:
+ log_debug(" (dropped permanent send error)");
+ f->f_dropped++;
f->f_type = F_UNUSED;
- log_warn("sendmsg to \"%s\"",
+ snprintf(ebuf, sizeof(ebuf),
+ "to udp loghost \"%s\"",
+ f->f_un.f_forw.f_loghost);
+ dropped_warn(>f_dropped, ebuf);
+ log_warn("loghost \"%s\" disabled, sendmsg",
f->f_un.f_forw.f_loghost);
break;
}
+ } else {
+ log_debug("");
+ if (f->f_dropped > 0) {
+ snprintf(ebuf, sizeof(ebuf),
+ "to udp loghost \"%s\"",
+ f->f_un.f_forw.f_loghost);
+ dropped_warn(>f_dropped, ebuf);
+ }
}
break;
@@ -2056,7 +2074,7 @@ fprintlog(struct filed *f, int flags, ch
log_debugadd(" %s", f->f_un.f_forw.f_loghost);
if (EVBUFFER_LENGTH(f->f_un.f_forw.f_bufev->output) >=
MAX_TCPBUF) {
- log_debug(" (dropped)");
+ log_debug(" (dropped tcpbuf full)");
f->f_dropped++;
break;
}
@@ -2077,12 +2095,12 @@ fprintlog(struct filed *f, int flags, ch
(char *)iov[3].iov_base, (char *)iov[4].iov_base,
(char *)iov[5].iov_base, (char *)iov[6].iov_base);
if (l < 0) {
- log_debug(" (dropped evbuffer_add_printf)");
+ log_debug(" (dropped evbuffer add)");
f->f_dropped++;
Re: HAMMER2 filesystem for OpenBSD
On Tue, 2023-10-17 at 16:22 -0600, Theo de Raadt wrote: > What will come of this discussion of opinions? If it's doable and the code quality is acceptable, I see 2 outcomes: 1. I could start looking for sponsorship, as I feel like my level of expertise is too limited to chew it myself 2. perhaps with a right guidance I could try to bite it myself (not so sure about it, but keen on finding out). I'm trying to contact the author atm to discuss his position and his plans regarding the code so see if there is any alignment. Week ago, he added a disclaimer in the readme that he plans shift his focus when FreeBSD write support is stabilized. If there is a limited window of opportunity, I thought it's worth looking at it while there is active interest. TBH, I don't know what would it take. The patch looks minimal, but I suspect it's just tip of the iceberg and I lack the knowledge to estimate how big the underwater part is. Especially the cost of maintenance. Therefore, I decided to reach out to tech, kindly asking about devs opinion or technical expertise on that matter. If such effort is not aligned with OpenBSD plans, I'll stop here. Cheers, Chris
Re: HAMMER2 filesystem for OpenBSD
What will come of this discussion of opinions? Chris Narkiewicz wrote: > Hi, > > Tomohiro Kusumi is currently working on HAMMER2 implementation > for OpenBSD, FreeBSD and NetBSD. > > The repository is here: > https://github.com/kusumi/openbsd_hammer2 > > > He maintains repositories for NetBSD, FreeBSD and OpenBSD, which > suggests that the implementation is portable. He also > provides a patch for OpenBSD 7.3: > > https://github.com/kusumi/openbsd_hammer2/blob/master/patch/openbsd73.patch > > The patch looks very minimal to me, with no deeper changes to the > kernel. > > I haven't found any discussion about HAMMER2 in list archives, so I'd > like to bring it to devs attention, kindly asking for your opinion. > Does it look like it's worth bringing in? Does it require more work? > > I'd appreciate any opinions from more knowledgable crowd. > > Cheers, > Chris >
HAMMER2 filesystem for OpenBSD
Hi, Tomohiro Kusumi is currently working on HAMMER2 implementation for OpenBSD, FreeBSD and NetBSD. The repository is here: https://github.com/kusumi/openbsd_hammer2 He maintains repositories for NetBSD, FreeBSD and OpenBSD, which suggests that the implementation is portable. He also provides a patch for OpenBSD 7.3: https://github.com/kusumi/openbsd_hammer2/blob/master/patch/openbsd73.patch The patch looks very minimal to me, with no deeper changes to the kernel. I haven't found any discussion about HAMMER2 in list archives, so I'd like to bring it to devs attention, kindly asking for your opinion. Does it look like it's worth bringing in? Does it require more work? I'd appreciate any opinions from more knowledgable crowd. Cheers, Chris
Re: smtpd: implement nullmx RFC 7505
[2023-10-17 17:32] Omar Polo > sorry for the terrifc delay. > > On 2023/10/01 14:59:15 +0200, Philipp wrote: > > Hi > > > > Setting Null MX is a way for domainowners to indicate that the domain > > does not accept mail. Currently a Null MX causes a tempfail and the > > mail will be queued and tried to resubmitted till a timeout. With the > > attached patch a Null MX causes a permfail. This way the sender will > > directly get a bounce with the message "Domain does not accept mail". > > I'm not sure how much widespread the usage is, but it doesn't seem a > bad feature. It's simple to implement and it provides some (very > small) value. There is a blogpost[0] which says about 1% of the domains use Null MX and about 0.7% set the MX to "localhost." (probably for the same propose). > In general I'm OK with the idea, but would need some OKs from other > developers too. > > There is one part of the RFC7505 that I'd like to quote and discuss > with you however. The last paragraph of the section 3 says: > > : A domain that advertises a null MX MUST NOT advertise any other MX > : RR. > > Your implementation handles the case where there are more than one MX > by setting the `nullmx' field in the shared struct when we encounter > one, and then don't process the other MXs when that field is set. > This is fine. > > However, I think we can simplify here by not honouring the Null MX > when there are other MX records. We're not violating the RFC. See > diff below to see what I mean. The only difference is in dns.c, the > rest is the same with yours. My reasaning for that was: When you set Null MX you explicitly opt out from reciving mail even when you violate the spec. But if you want it diffrent I can change it. But I don't think your proposed patch is a good solution, because the result depend on the order of the RR in the repsonse. The problem is when the first entry in the response is a Null MX your patch truncate the other results and a bounce is generated. But when the Null MX is later in the response the other entries are used to deliver the mail. > So far I've only compile-tested the code. I don't have a spare domain > to test and don't know of anyone using a Null MX. To test Null MX you can use example.com. To test the localhost patch a friend of me has set up mxtest.yannikenss.de. > > Because some domains set the MX record to "localhost." to get a similar > > efect the secound patch ignores "localhost." MX entries and handles a MX > > containing only "localhost." records like a Null MX. > > This could be simplified too. On top of diff below, it would need > just something among the lines of: When localhost and Null MX should be handled the same this could be simplified in one check, yes. But that "localhost." and Null MX are handled different was on purpose. Because I would say setting a Null MX is a explicit opt out from reciving mail and setting MX to localhost is implicit. As said before, I have no problem which changing this. > I'm still not convinced about this part however. It feels wrong to me > to hardcode such a check, it seems too much paranoic. The follow-up > would be to check for localhost in dns_dispatch_host too? ;) The idea[1] about this part was because setting "localhost." as MX is used in a similiar way as Null MX[0]. So handle this in a simmilar way as Null MX make sense. This way the sender will get a better bounce message. Philipp [0] https://www.netmeister.org/blog/mx-diversity.html [1] I still belive OpenSMTPD should only connect to public routable addresses for relay. So don't connect to something like 127.0.0.1/8, ::1/128, RFC1918 addresses, ULA, ... But this is independent of this patch.
Re: snmpd [3/16]: Distinguish between parseerror and openfailed for agentx-open-pdu
On Tue, Oct 17, 2023 at 02:51:03PM +0200, Martijn van Duren wrote: > RFC2741 section 7.1.1 tells us that if a pdu can't be parsed we must > return a parseerror. Section 7.1 gives an example of "An unrecognized > value is encountered". The spec is vague is a bit vague on what > constitutes a parseerror, vs a protocol error, so I don't want to > muddle too much with that part, but let's at least return an > appropriate error when a client sends invalid data in the open request. > > The spec clearly states that any error, which is not a parseerror must > be a openfailed error. So no processingerrors. > > OK? ok tb
Re: snmpd [1.1/16]: Don't overflow oid in agentx parser
On Tue, Oct 17, 2023 at 02:49:05PM +0200, Martijn van Duren wrote: > > Currently ax.c doesn't check the maximum length of an OID ax_pdutooid. > > This can lead to a buffer overflow. Even though it must be fixed, I > > don't think there's a big risk here, since an attacker would need to have > > access to the agentx socket, which by default is disabled and defaults > > to root:_agentx when enabled. > > Here's the libagentx counterpart. ok for both
Re: snmpd [2.2/16]: Don't set NON_DEFAULT_CONTEXT for agentx-response-pdu
On Tue, Oct 17, 2023 at 02:49:57PM +0200, Martijn van Duren wrote: > > According to RFC2741 section 6.1.1 an agentx-response-pdu shouldn't have > > the NON_DEFAULT_CONTEXT set. Remove the argument from ax_response(). > > Here's the libagentx counterpart. ok
Re: smtpd: implement nullmx RFC 7505
Hello,
sorry for the terrifc delay.
On 2023/10/01 14:59:15 +0200, Philipp wrote:
> Hi
>
> Setting Null MX is a way for domainowners to indicate that the domain
> does not accept mail. Currently a Null MX causes a tempfail and the
> mail will be queued and tried to resubmitted till a timeout. With the
> attached patch a Null MX causes a permfail. This way the sender will
> directly get a bounce with the message "Domain does not accept mail".
I'm not sure how much widespread the usage is, but it doesn't seem a
bad feature. It's simple to implement and it provides some (very
small) value.
In general I'm OK with the idea, but would need some OKs from other
developers too.
There is one part of the RFC7505 that I'd like to quote and discuss
with you however. The last paragraph of the section 3 says:
: A domain that advertises a null MX MUST NOT advertise any other MX
: RR.
Your implementation handles the case where there are more than one MX
by setting the `nullmx' field in the shared struct when we encounter
one, and then don't process the other MXs when that field is set.
This is fine.
However, I think we can simplify here by not honouring the Null MX
when there are other MX records. We're not violating the RFC. See
diff below to see what I mean. The only difference is in dns.c, the
rest is the same with yours.
So far I've only compile-tested the code. I don't have a spare domain
to test and don't know of anyone using a Null MX.
> Because some domains set the MX record to "localhost." to get a similar
> efect the secound patch ignores "localhost." MX entries and handles a MX
> containing only "localhost." records like a Null MX.
This could be simplified too. On top of diff below, it would need
just something among the lines of:
- if (rr.rr.mx.preference == 0 && !strcmp(buf, "")) {
+ if ((rr.rr.mx.preference == 0 && !strcmp(buf, "")) ||
+ !strcasecmp(buf, "localhost")) {
if (found)
continue;
m_create(s->p, IMSG_MTA_DNS_HOST_END, 0, 0, -1);
I'm still not convinced about this part however. It feels wrong to me
to hardcode such a check, it seems too much paranoic. The follow-up
would be to check for localhost in dns_dispatch_host too? ;)
Thanks,
Omar Polo
diff 2d025d839f99dc09ee525c11a4ed09a0f3bbe7d0
8d6138e5b1e0bc112ff2584d8528e6bc95a39b6f
commit - 2d025d839f99dc09ee525c11a4ed09a0f3bbe7d0
commit + 8d6138e5b1e0bc112ff2584d8528e6bc95a39b6f
blob - 4cf5d23d1d14b5400c6f4429dae0a4f6490073d4
blob + effc07d142895fb2b622242efcd5c69da7f3b67c
--- usr.sbin/smtpd/dns.c
+++ usr.sbin/smtpd/dns.c
@@ -259,8 +259,22 @@ dns_dispatch_mx(struct asr_result *ar, void *arg)
unpack_rr(, );
if (rr.rr_type != T_MX)
continue;
+
print_dname(rr.rr.mx.exchange, buf, sizeof(buf));
buf[strlen(buf) - 1] = '\0';
+
+ if (rr.rr.mx.preference == 0 && !strcmp(buf, "")) {
+ if (found)
+ continue;
+ m_create(s->p, IMSG_MTA_DNS_HOST_END, 0, 0, -1);
+ m_add_id(s->p, s->reqid);
+ m_add_int(s->p, DNS_NULLMX);
+ m_close(s->p);
+ free(s);
+ found++;
+ break;
+ }
+
dns_lookup_host(s, buf, rr.rr.mx.preference);
found++;
}
blob - c0d0fc02931b90409441035d2744923af9e42df1
blob + 25e158b68a88c8485428a2476c1c557f8c60404d
--- usr.sbin/smtpd/mta.c
+++ usr.sbin/smtpd/mta.c
@@ -1088,6 +1088,10 @@ mta_on_mx(void *tag, void *arg, void *data)
else
relay->failstr = "No MX found for domain";
break;
+ case DNS_NULLMX:
+ relay->fail = IMSG_MTA_DELIVERY_PERMFAIL;
+ relay->failstr = "Domain does not accept mail";
+ break;
default:
fatalx("bad DNS lookup error code");
break;
blob - 6781286928da45e6159bce81ff2437011ebdef72
blob + 9f4732d1c27f6ef9f62951f0207f209a83038dcf
--- usr.sbin/smtpd/smtpd.h
+++ usr.sbin/smtpd/smtpd.h
@@ -1026,6 +1026,8 @@ enum dns_error {
DNS_EINVAL,
DNS_ENONAME,
DNS_ENOTFOUND,
+ /* rfc 7505 */
+ DNS_NULLMX,
};
enum lka_resp_status {
Re: Please test: make ipsec(4) timeouts mpsafe
On 17.10.2023. 1:07, Vitaliy Makkoveev wrote: >> On 13 Oct 2023, at 18:40, Hrvoje Popovski wrote: >> >> On 12.10.2023. 20:10, Vitaliy Makkoveev wrote: >>> Hi, MP safe process timeouts were landed to the tree, so time to test >>> them with network stack :) Diff below makes tdb and ids garbage >>> collector timeout handlers running without kernel lock. Not for commit, >>> just share this for tests if someone interesting. >> >> Hi, >> >> with this diff it seems that it's little slower than without it. >> 165Kpps with diff >> 200Kpps without diff >> > > Hi, > > Thanks for testing. I’m interesting on slower results. I suspect > enabled/disabled POOL_DEBUG effect. The patched and unpatched builds > were made from the same sources? Hi, I'm running same source with and without diff and with kern.pool_debug=0. I've tried few times and sometimes I'm getting same results, but mostly little slower ... I have 2 same servers directly connected with 10G x540T an one ipsec tunnel through that 10G interface. I'm testing like this: compile kernel from source - test tunnel apply your diff with same source - test tunnel
Re: bgpd cleanup around mask2prefixlen
On Tue, Oct 17, 2023 at 04:47:36PM +0200, Claudio Jeker wrote: > Looking at fixing portable I realized that some bits around mask2prefixlen > can be cleaned up. > > First in session.c the plen != 0xff check is not needed since it never can > happen. > > 2nd the checks for sin_len and sin6_len == 0 are also impossible. A > sockaddr can not have length 0. A default route would at least have > sin_len == 4 but on OpenBSD even that no longer happens. Yes, this looks like a mistake must have happened earlier. The only realistic scenario would be an addr that comes freshly out of memset(). But for those nothing changes and the return 0 remains. > Last check for RTF_HOST first, then for sa_in != NULL. If RTF_HOST is set > the netmask is irrelevant. ok tb.
bgpd cleanup around mask2prefixlen
Looking at fixing portable I realized that some bits around mask2prefixlen
can be cleaned up.
First in session.c the plen != 0xff check is not needed since it never can
happen.
2nd the checks for sin_len and sin6_len == 0 are also impossible. A
sockaddr can not have length 0. A default route would at least have
sin_len == 4 but on OpenBSD even that no longer happens.
Last check for RTF_HOST first, then for sa_in != NULL. If RTF_HOST is set
the netmask is irrelevant.
--
:wq Claudio
Index: kroute.c
===
RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
retrieving revision 1.306
diff -u -p -r1.306 kroute.c
--- kroute.c16 Oct 2023 10:25:45 - 1.306
+++ kroute.c17 Oct 2023 13:18:42 -
@@ -2422,8 +2422,6 @@ mask2prefixlen4(struct sockaddr_in *sa_i
{
in_addr_t ina;
- if (sa_in->sin_len == 0)
- return (0);
ina = sa_in->sin_addr.s_addr;
if (ina == 0)
return (0);
@@ -2437,8 +2435,6 @@ mask2prefixlen6(struct sockaddr_in6 *sa_
uint8_t *ap, *ep;
u_intl = 0;
- if (sa_in6->sin6_len == 0)
- return (0);
/*
* sin6_len is the size of the sockaddr so subtract the offset of
* the possibly truncated sin6_addr struct.
@@ -3096,20 +3092,20 @@ dispatch_rtmsg_addr(struct rt_msghdr *rt
switch (sa->sa_family) {
case AF_INET:
sa_in = (struct sockaddr_in *)rti_info[RTAX_NETMASK];
- if (sa_in != NULL) {
- kf->prefixlen = mask2prefixlen4(sa_in);
- } else if (rtm->rtm_flags & RTF_HOST)
+ if (rtm->rtm_flags & RTF_HOST)
kf->prefixlen = 32;
+ else if (sa_in != NULL)
+ kf->prefixlen = mask2prefixlen4(sa_in);
else
kf->prefixlen =
prefixlen_classful(kf->prefix.v4.s_addr);
break;
case AF_INET6:
sa_in6 = (struct sockaddr_in6 *)rti_info[RTAX_NETMASK];
- if (sa_in6 != NULL) {
- kf->prefixlen = mask2prefixlen6(sa_in6);
- } else if (rtm->rtm_flags & RTF_HOST)
+ if (rtm->rtm_flags & RTF_HOST)
kf->prefixlen = 128;
+ else if (sa_in6 != NULL)
+ kf->prefixlen = mask2prefixlen6(sa_in6);
else
fatalx("in6 net addr without netmask");
break;
Index: session.c
===
RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
retrieving revision 1.449
diff -u -p -r1.449 session.c
--- session.c 16 Oct 2023 10:25:46 - 1.449
+++ session.c 16 Oct 2023 17:14:41 -
@@ -1241,8 +1241,7 @@ get_alternate_addr(struct bgpd_addr *loc
plen = mask2prefixlen(
match->ifa_addr->sa_family,
match->ifa_netmask);
- if (plen != 0xff &&
- prefix_compare(local, remote, plen) == 0)
+ if (prefix_compare(local, remote, plen) == 0)
connected = 1;
}
break;
snmpd [16/16]: Keep track of exact registrations via AgentX
This diff I'm least convinced about, but I do want to put it out there.
As far as I'm aware RFC2741 places no restrictions on direct mapping of
unregistrations on registrations. Meaning that if I register
1.3.6.1.2.[1-10] and I just unregister 1.3.6.1.2.5 this should leave
1.3.6.1.2.[1-4] and 1.3.6.1.2.[6-10] and this is what currently is
implemented (assuming previous (un)register diffs).
However, RFC2742 specifies AgentxRegistrationEntry, which presents
the the data as presented in the agentx-register-pdu. This implies
that an unregister is expected to be an exact counterpart to
agentx-register-pdu, because else an AgentxRegistrationEntry would
need to be split up, which would be a horror-show.
Do we want to add this restriction and keep the road open to
AGENTX-MIB support? Since range support is currently broken anyway
this won't cause any problems.
Thoughts? OK?
martijn@
diff --git a/application_agentx.c b/application_agentx.c
index 79900d6..60dc4df 100644
--- a/application_agentx.c
+++ b/application_agentx.c
@@ -70,11 +70,28 @@ struct appl_agentx_session {
struct ax_oid sess_oid;
struct ax_ostring sess_descr;
struct appl_backend sess_backend;
+ TAILQ_HEAD(, appl_agentx_registration) sess_registrations;
RB_ENTRY(appl_agentx_session) sess_entry;
TAILQ_ENTRY(appl_agentx_session) sess_conn_entry;
};
+/* RFC2742: AGENTX-MIB:AgentxRegistrationEntry */
+struct appl_agentx_registration {
+ uint32_t reg_index;
+ char reg_context[APPL_CONTEXTNAME_MAX + 1];
+ struct ax_oid reg_start;
+ uint8_t reg_rangesubid;
+ uint32_t reg_upperbound;
+ uint8_t reg_priority;
+ uint8_t reg_timeout;
+ uint8_t reg_instance;
+
+ struct appl_agentx_session *reg_session;
+
+ TAILQ_ENTRY(appl_agentx_registration) reg_entry;
+};
+
void appl_agentx_listen(struct agentx_master *);
void appl_agentx_accept(int, short, void *);
void appl_agentx_free(struct appl_agentx_connection *, enum appl_close_reason);
@@ -106,6 +123,7 @@ struct appl_backend_functions appl_agentx_functions = {
.ab_getnext = appl_agentx_getnext,
.ab_getbulk = NULL, /* not properly supported in application.c and
libagentx */
};
+static uint32_t appl_agentx_reg_index = 1;
RB_HEAD(appl_agentx_conns, appl_agentx_connection) appl_agentx_conns =
RB_INITIALIZER(_agentx_conns);
@@ -477,6 +495,7 @@ appl_agentx_open(struct appl_agentx_connection *conn,
struct ax_pdu *pdu)
goto fail;
}
session->sess_descr.aos_string = NULL;
+ TAILQ_INIT(&(session->sess_registrations));
session->sess_conn = conn;
if (pdu->ap_header.aph_flags & AX_PDU_FLAG_NETWORK_BYTE_ORDER)
@@ -624,6 +643,12 @@ void
appl_agentx_session_free(struct appl_agentx_session *session)
{
struct appl_agentx_connection *conn = session->sess_conn;
+ struct appl_agentx_registration *reg;
+
+ while ((reg = TAILQ_FIRST(&(session->sess_registrations))) != NULL) {
+ TAILQ_REMOVE(&(session->sess_registrations), reg, reg_entry);
+ free(reg);
+ }
appl_close(&(session->sess_backend));
@@ -642,6 +667,7 @@ appl_agentx_register(struct appl_agentx_session *session,
struct ax_pdu *pdu)
struct ber_oid oid;
enum appl_error error;
int subtree = 0;
+ struct appl_agentx_registration *reg;
timeout = pdu->ap_payload.ap_register.ap_timeout;
timeout = timeout != 0 ? timeout : session->sess_timeout != 0 ?
@@ -660,6 +686,13 @@ appl_agentx_register(struct appl_agentx_session *session,
struct ax_pdu *pdu)
goto fail;
}
+ if ((reg = malloc(sizeof(*reg))) == NULL) {
+ log_warn("%s: Failed to register",
+ session->sess_backend.ab_name);
+ error = APPL_ERROR_PROCESSINGERROR;
+ goto fail;
+ }
+
error = appl_register(pdu->ap_context.aos_string, timeout,
pdu->ap_payload.ap_register.ap_priority, ,
pdu->ap_header.aph_flags & AX_PDU_FLAG_INSTANCE_REGISTRATION,
@@ -667,6 +700,26 @@ appl_agentx_register(struct appl_agentx_session *session,
struct ax_pdu *pdu)
pdu->ap_payload.ap_register.ap_upper_bound,
&(session->sess_backend));
+ if (error == APPL_ERROR_NOERROR) {
+ reg->reg_index = appl_agentx_reg_index++;
+ (void)strlcpy(reg->reg_context, pdu->ap_context.aos_slen == 0 ?
+ "" : (const char *)pdu->ap_context.aos_string,
+ sizeof(reg->reg_context));
+ reg->reg_start = pdu->ap_payload.ap_register.ap_subtree;
+ reg->reg_rangesubid =
+ pdu->ap_payload.ap_register.ap_range_subid;
+ reg->reg_upperbound =
+ pdu->ap_payload.ap_register.ap_upper_bound;
+ reg->reg_priority = pdu->ap_payload.ap_register.ap_priority;
+
snmpd [15/16]: When we have an error, all oids must be identical to the request
RFC3416 section 4.2.1 (and others) tells us that if an error occurs the
varbindlist in the response must be identical to the original request.
There might be other edge-cases here, but let's at least make sure that
the OIDs haven't been tampered with.
OK?
martijn@
diff --git a/application.c b/application.c
index 53e793d..d98918f 100644
--- a/application.c
+++ b/application.c
@@ -1186,6 +1186,9 @@ appl_varbind_valid(struct appl_varbind *varbind,
int cmp;
int eomv = 0;
+ if (null)
+ next = 0;
+
if (varbind->av_value == NULL) {
if (!null) {
*errstr = "missing value";
snmpd [14/16]: Validate the returned error code
Certain error codes are only intended for certain request-types. Add an
appl_error_valid() function to test for this.
OK?
martijn@
diff --git a/application.c b/application.c
index 6ddeb39..53e793d 100644
--- a/application.c
+++ b/application.c
@@ -130,6 +130,7 @@ void appl_request_downstream_timeout(int, short, void *);
void appl_request_upstream_reply(struct appl_request_upstream *);
int appl_varbind_valid(struct appl_varbind *, struct appl_varbind_internal *,
int, int, int, const char **);
+int appl_error_valid(enum appl_error, enum snmp_pdutype);
int appl_varbind_backend(struct appl_varbind_internal *);
void appl_varbind_error(struct appl_varbind_internal *, enum appl_error);
void appl_report(struct snmp_message *, int32_t, struct ber_oid *,
@@ -1069,6 +1070,11 @@ appl_response(struct appl_backend *backend, int32_t
requestid,
next = pdutype == SNMP_C_GETNEXTREQ ||
pdutype == SNMP_C_GETBULKREQ;
origvb = dreq->ard_vblist;
+ if (!appl_error_valid(error, dreq->ard_requesttype)) {
+ log_warnx("%s: %"PRIu32" Invalid error",
+ backend->ab_name, requestid);
+ invalid = 1;
+ }
}
vb = vblist;
@@ -1295,6 +1301,37 @@ appl_varbind_valid(struct appl_varbind *varbind,
return 1;
}
+int
+appl_error_valid(enum appl_error error, enum snmp_pdutype type)
+{
+ switch (error) {
+ case APPL_ERROR_NOERROR:
+ case APPL_ERROR_TOOBIG:
+ case APPL_ERROR_NOSUCHNAME:
+ case APPL_ERROR_GENERR:
+ return 1;
+ case APPL_ERROR_BADVALUE:
+ case APPL_ERROR_READONLY:
+ case APPL_ERROR_NOACCESS:
+ case APPL_ERROR_WRONGTYPE:
+ case APPL_ERROR_WRONGLENGTH:
+ case APPL_ERROR_WRONGENCODING:
+ case APPL_ERROR_WRONGVALUE:
+ case APPL_ERROR_NOCREATION:
+ case APPL_ERROR_INCONSISTENTVALUE:
+ case APPL_ERROR_RESOURCEUNAVAILABLE:
+ case APPL_ERROR_COMMITFAILED:
+ case APPL_ERROR_UNDOFAILED:
+ case APPL_ERROR_NOTWRITABLE:
+ case APPL_ERROR_INCONSISTENTNAME:
+ return type == SNMP_C_SETREQ;
+ case APPL_ERROR_AUTHORIZATIONERROR:
+ return type == SNMP_C_GETREQ || type == SNMP_C_SETREQ;
+ default:
+ return 0;
+ }
+}
+
int
appl_varbind_backend(struct appl_varbind_internal *ivb)
{
snmpd [13/16]: registered instances must not return below OID
If a backend registers as an instance it must never return OIDs below
their registration. Add a test for this in appl_varbind_valid().
OK?
martijn@
diff --git a/application.c b/application.c
index dfa7220..6ddeb39 100644
--- a/application.c
+++ b/application.c
@@ -128,8 +128,8 @@ void appl_request_upstream_resolve(struct
appl_request_upstream *);
void appl_request_downstream_send(struct appl_request_downstream *);
void appl_request_downstream_timeout(int, short, void *);
void appl_request_upstream_reply(struct appl_request_upstream *);
-int appl_varbind_valid(struct appl_varbind *, struct appl_varbind *, int, int,
-int, const char **);
+int appl_varbind_valid(struct appl_varbind *, struct appl_varbind_internal *,
+int, int, int, const char **);
int appl_varbind_backend(struct appl_varbind_internal *);
void appl_varbind_error(struct appl_varbind_internal *, enum appl_error);
void appl_report(struct snmp_message *, int32_t, struct ber_oid *,
@@ -1073,9 +1073,9 @@ appl_response(struct appl_backend *backend, int32_t
requestid,
vb = vblist;
for (i = 1; vb != NULL; vb = vb->av_next, i++) {
-if (!appl_varbind_valid(vb, origvb == NULL ?
- NULL : &(origvb->avi_varbind), next,
-error != APPL_ERROR_NOERROR, backend->ab_range, )) {
+if (!appl_varbind_valid(vb, origvb == NULL ? NULL : origvb,
+ next, error != APPL_ERROR_NOERROR, backend->ab_range,
+ )) {
smi_oid2string(&(vb->av_oid), oidbuf,
sizeof(oidbuf), 0);
log_warnx("%s: %"PRIu32" %s: %s",
@@ -1173,8 +1173,9 @@ appl_response(struct appl_backend *backend, int32_t
requestid,
}
int
-appl_varbind_valid(struct appl_varbind *varbind, struct appl_varbind *request,
-int next, int null, int range, const char **errstr)
+appl_varbind_valid(struct appl_varbind *varbind,
+struct appl_varbind_internal *request, int next, int null, int range,
+const char **errstr)
{
int cmp;
int eomv = 0;
@@ -1259,24 +1260,32 @@ appl_varbind_valid(struct appl_varbind *varbind, struct
appl_varbind *request,
if (request == NULL)
return 1;
- cmp = ober_oid_cmp(&(request->av_oid), &(varbind->av_oid));
- if (next && !eomv) {
- if (request->av_include) {
- if (cmp > 0) {
- *errstr = "oid not incrementing";
- return 0;
+ cmp = ober_oid_cmp(&(request->avi_varbind.av_oid), &(varbind->av_oid));
+ if (next) {
+ if (request->avi_region->ar_instance &&
+ ober_oid_cmp(&(request->avi_region->ar_oid),
+ &(varbind->av_oid)) != 0) {
+ *errstr = "oid below instance";
+ return 0;
+ }
+ if (!eomv) {
+ if (request->avi_varbind.av_include) {
+ if (cmp > 0) {
+ *errstr = "oid not incrementing";
+ return 0;
+ }
+ } else {
+ if (cmp >= 0) {
+ *errstr = "oid not incrementing";
+ return 0;
+ }
}
- } else {
- if (cmp >= 0) {
- *errstr = "oid not incrementing";
+ if (range && ober_oid_cmp(&(varbind->av_oid),
+ &(request->avi_varbind.av_oid_end)) > 0) {
+ *errstr = "end oid not honoured";
return 0;
}
}
- if (range && ober_oid_cmp(&(varbind->av_oid),
- &(request->av_oid_end)) > 0) {
- *errstr = "end oid not honoured";
- return 0;
- }
} else {
if (cmp != 0) {
*errstr = "oids not equal";
snmpd [12/16]: Make ab_range in application_agentx explicit 1
appl_agentx_session doesn't set ab_range explicitly to 1, and thus relies on malloc randomness to set it. Sit it explicitly. OK? martijn@ diff --git a/application_agentx.c b/application_agentx.c index 680725d..79900d6 100644 --- a/application_agentx.c +++ b/application_agentx.c @@ -548,6 +548,7 @@ appl_agentx_open(struct appl_agentx_connection *conn, struct ax_pdu *pdu) session->sess_backend.ab_cookie = session; session->sess_backend.ab_retries = 0; session->sess_backend.ab_fn = _agentx_functions; + session->sess_backend.ab_range = 1; RB_INIT(&(session->sess_backend.ab_requests)); TAILQ_INSERT_TAIL(&(conn->conn_sessions), session, sess_conn_entry);
snmpd [11/16]: When a request results in EOMV we must return the requesting OID
According to RFC3416 section 4.2.2 and 4.2.3 case "(2)" when an
endOfMibView is returned the OID must be identical to originally
requested OID. Currently this can fail when the original request
is in a !last registered region and all subsequent regions also
return EOMV.
Store the original OID and use that on EOMV.
OK?
martijn@
diff --git a/application.c b/application.c
index e780025..dfa7220 100644
--- a/application.c
+++ b/application.c
@@ -99,6 +99,7 @@ struct appl_varbind_internal {
enum appl_varbind_state avi_state;
struct appl_varbind avi_varbind;
struct appl_region *avi_region;
+ struct ber_oid avi_origid;
int16_t avi_index;
struct appl_request_upstream *avi_request_upstream;
struct appl_request_downstream *avi_request_downstream;
@@ -679,6 +680,8 @@ appl_processpdu(struct snmp_message *statereference, const
char *ctxname,
}
ober_get_oid(varbind->be_sub,
&(ureq->aru_vblist[i].avi_varbind.av_oid));
+ ureq->aru_vblist[i].avi_origid =
+ ureq->aru_vblist[i].avi_varbind.av_oid;
if (i + 1 < ureq->aru_varbindlen) {
ureq->aru_vblist[i].avi_next =
&(ureq->aru_vblist[i + 1]);
@@ -1008,6 +1011,10 @@ appl_request_upstream_reply(struct appl_request_upstream
*ureq)
vb = &(ureq->aru_vblist[i]);
vb->avi_varbind.av_next =
&(ureq->aru_vblist[i + 1].avi_varbind);
+ value = vb->avi_varbind.av_value;
+ if (value->be_class == BER_CLASS_CONTEXT &&
+ value->be_type == APPL_EXC_ENDOFMIBVIEW)
+ vb->avi_varbind.av_oid = vb->avi_origid;
}
ureq->aru_vblist[i - 1].avi_varbind.av_next = NULL;
snmpd [10/16]: Make retries on open session where connection is closed return early
Here's a special case unlikely to be found in the wild: When opening 2 sessions on an agentx connection (already unusual) and registering 2 overlapping regions on the different sessions, e.g. by differing in priority (even more unusual) and we close the underlying connection with an outstanding request to the dominant region we will call appl_agentx_free(), which sequentially closes all sessions. If the session with the outstanding request is closed before the second session the request is retried before said session is cleaned up and it will try to send it over a conn_ax which at that point has been set to NULL, resulting in a SIGSEGV. Simply return early and let this second request be cancelled by the cleanup of the second session. OK? martijn@ diff --git a/application_agentx.c b/application_agentx.c index 2231d4c..680725d 100644 --- a/application_agentx.c +++ b/application_agentx.c @@ -712,6 +712,9 @@ appl_agentx_get(struct appl_backend *backend, int32_t transactionid, struct ax_searchrange *srl; size_t i, j, nsr; + if (session->sess_conn->conn_ax == NULL) + return; + for (nsr = 0, vb = vblist; vb != NULL; vb = vb->av_next) nsr++; @@ -760,6 +763,9 @@ appl_agentx_getnext(struct appl_backend *backend, int32_t transactionid, struct ax_searchrange *srl; size_t i, j, nsr; + if (session->sess_conn->conn_ax == NULL) + return; + for (nsr = 0, vb = vblist; vb != NULL; vb = vb->av_next) nsr++;
snmpd [9/16]: Fix range handling with appl_unregister
Right now (un)registering a region with range_subid set to !0 will
fail. Apparently nothing in the wild uses this, but let's fix it.
This is the unregister part.
OK? martijn@
diff --git a/application.c b/application.c
index f8709b8..e780025 100644
--- a/application.c
+++ b/application.c
@@ -115,6 +115,8 @@ struct snmp_target_mib {
enum appl_error appl_region(struct appl_context *, uint32_t, uint8_t,
struct ber_oid *, int, int, struct appl_backend *);
void appl_region_free(struct appl_context *, struct appl_region *);
+enum appl_error appl_region_unregister_match(struct appl_context *, uint8_t,
+struct ber_oid *, char *, struct appl_backend *, int);
struct appl_region *appl_region_find(struct appl_context *,
const struct ber_oid *);
struct appl_region *appl_region_next(struct appl_context *,
@@ -400,9 +402,10 @@ enum appl_error
appl_unregister(const char *ctxname, uint8_t priority, struct ber_oid *oid,
uint8_t range_subid, uint32_t upper_bound, struct appl_backend *backend)
{
- struct appl_region *region, search;
struct appl_context *ctx;
char oidbuf[1024], subidbuf[11];
+ enum appl_error error;
+ uint32_t lower_bound;
size_t i;
oidbuf[0] = '\0';
@@ -444,34 +447,45 @@ appl_unregister(const char *ctxname, uint8_t priority,
struct ber_oid *oid,
return APPL_ERROR_PARSEERROR;
}
- if (range_subid > oid->bo_n) {
+ if (range_subid == 0)
+ return appl_region_unregister_match(ctx, priority, oid, oidbuf,
+ backend, 1);
+
+ range_subid--;
+ if (range_subid >= oid->bo_n) {
log_warnx("%s: Can't unregiser %s: range_subid too large",
backend->ab_name, oidbuf);
return APPL_ERROR_PARSEERROR;
}
- if (range_subid != 0 && oid->bo_id[range_subid] >= upper_bound) {
- log_warnx("%s: Can't unregister %s: upper bound smaller or "
- "equal to range_subid", backend->ab_name, oidbuf);
+ if (oid->bo_id[range_subid] > upper_bound) {
+ log_warnx("%s: Can't unregister %s: upper bound smaller than "
+ "range_subid", backend->ab_name, oidbuf);
return APPL_ERROR_PARSEERROR;
}
+ lower_bound = oid->bo_id[range_subid];
+ do {
+ if ((error = appl_region_unregister_match(ctx, priority, oid,
+ oidbuf, backend, 0)) != APPL_ERROR_NOERROR)
+ return error;
+ } while (oid->bo_id[range_subid]++ != upper_bound);
+
+ oid->bo_id[range_subid] = lower_bound;
+ do {
+ (void)appl_region_unregister_match(ctx, priority, oid, oidbuf,
+ backend, 1);
+ } while (oid->bo_id[range_subid]++ != upper_bound);
+
+ return APPL_ERROR_NOERROR;
+}
+
+enum appl_error
+appl_region_unregister_match(struct appl_context *ctx, uint8_t priority,
+struct ber_oid *oid, char *oidbuf, struct appl_backend *backend, int
dofree)
+{
+ struct appl_region *region, search;
+
search.ar_oid = *oid;
- while (range_subid != 0 &&
- search.ar_oid.bo_id[range_subid] != upper_bound) {
- region = RB_FIND(appl_regions, &(ctx->ac_regions), );
- while (region != NULL && region->ar_priority < priority)
- region = region->ar_next;
- if (region == NULL || region->ar_priority != priority) {
- log_warnx("%s: Can't unregister %s: region not found",
- backend->ab_name, oidbuf);
- return APPL_ERROR_UNKNOWNREGISTRATION;
- }
- if (region->ar_backend != backend) {
- log_warnx("%s: Can't unregister %s: region not owned "
- "by backend", backend->ab_name, oidbuf);
- return APPL_ERROR_UNKNOWNREGISTRATION;
- }
- }
region = RB_FIND(appl_regions, &(ctx->ac_regions), );
while (region != NULL && region->ar_priority < priority)
region = region->ar_next;
@@ -485,20 +499,8 @@ appl_unregister(const char *ctxname, uint8_t priority,
struct ber_oid *oid,
"by backend", backend->ab_name, oidbuf);
return APPL_ERROR_UNKNOWNREGISTRATION;
}
-
- search.ar_oid = *oid;
- while (range_subid != 0 &&
- search.ar_oid.bo_id[range_subid] != upper_bound) {
- region = RB_FIND(appl_regions, &(ctx->ac_regions), );
- while (region != NULL && region->ar_priority != priority)
- region = region->ar_next;
+ if (dofree)
appl_region_free(ctx, region);
- }
- region = RB_FIND(appl_regions, &(ctx->ac_regions), );
- while (region != NULL && region->ar_priority != priority)
- region = region->ar_next;
- appl_region_free(ctx,
snmpd [8/16]: Fix range handling with appl_register
Right now (un)registering a region with range_subid set to !0 will
fail. Apparently nothing in the wild uses this, but let's fix it.
This is the register part.
OK?
martijn@
diff --git a/application.c b/application.c
index a02260b..f8709b8 100644
--- a/application.c
+++ b/application.c
@@ -347,28 +347,29 @@ appl_register(const char *ctxname, uint32_t timeout,
uint8_t priority,
backend->ab_name, oidbuf);
return APPL_ERROR_PARSEERROR;
}
- if (range_subid > oid->bo_n) {
+
+ if (range_subid == 0)
+ return appl_region(ctx, timeout, priority, oid, instance,
+ subtree, backend);
+
+ range_subid--;
+ if (range_subid >= oid->bo_n) {
log_warnx("%s: Can't register %s: range_subid too large",
backend->ab_name, oidbuf);
return APPL_ERROR_PARSEERROR;
}
- if (range_subid != 0 && oid->bo_id[range_subid] >= upper_bound) {
- log_warnx("%s: Can't register %s: upper bound smaller or equal "
- "to range_subid", backend->ab_name, oidbuf);
+ if (oid->bo_id[range_subid] > upper_bound) {
+ log_warnx("%s: Can't register %s: upper bound smaller than "
+ "range_subid", backend->ab_name, oidbuf);
return APPL_ERROR_PARSEERROR;
}
- if (range_subid != 0)
- lower_bound = oid->bo_id[range_subid];
-
- if (range_subid == 0)
- return appl_region(ctx, timeout, priority, oid, instance,
- subtree, backend);
+ lower_bound = oid->bo_id[range_subid];
do {
if ((error = appl_region(ctx, timeout, priority, oid, instance,
subtree, backend)) != APPL_ERROR_NOERROR)
goto fail;
- } while (oid->bo_id[range_subid] != upper_bound);
+ } while (oid->bo_id[range_subid]++ != upper_bound);
if ((error = appl_region(ctx, timeout, priority, oid, instance, subtree,
backend)) != APPL_ERROR_NOERROR)
goto fail;
snmpd [7/16]: Treat agentx-close-pdu with reasonByManager as a parseerror
RFC2741 section 6.2.2 says that reasonByManager can only be used by the
agentx master. Treat this reason as a parseerror.
OK?
martijn@
diff --git a/application_agentx.c b/application_agentx.c
index d1efae2..2231d4c 100644
--- a/application_agentx.c
+++ b/application_agentx.c
@@ -576,16 +576,29 @@ appl_agentx_close(struct appl_agentx_session *session,
struct ax_pdu *pdu)
{
struct appl_agentx_connection *conn = session->sess_conn;
char name[100];
+ enum appl_error error = APPL_ERROR_NOERROR;
strlcpy(name, session->sess_backend.ab_name, sizeof(name));
- appl_agentx_session_free(session);
- log_info("%s: Closed by subagent (%s)", name,
- ax_closereason2string(pdu->ap_payload.ap_close.ap_reason));
+ if (pdu->ap_payload.ap_close.ap_reason == AX_CLOSE_BYMANAGER) {
+ log_warnx("%s: Invalid close reason", name);
+ error = APPL_ERROR_PARSEERROR;
+ } else {
+ appl_agentx_session_free(session);
+ log_info("%s: Closed by subagent (%s)", name,
+ ax_closereason2string(pdu->ap_payload.ap_close.ap_reason));
+ }
ax_response(conn->conn_ax, pdu->ap_header.aph_sessionid,
pdu->ap_header.aph_transactionid, pdu->ap_header.aph_packetid,
- smi_getticks(), APPL_ERROR_NOERROR, 0, NULL, 0);
+ smi_getticks(), error, 0, NULL, 0);
appl_agentx_send(-1, EV_WRITE, conn);
+ if (error == APPL_ERROR_NOERROR)
+ return;
+
+ appl_agentx_forceclose(&(session->sess_backend),
+ APPL_CLOSE_REASONPARSEERROR);
+ if (TAILQ_EMPTY(&(conn->conn_sessions)))
+ appl_agentx_free(conn, APPL_CLOSE_REASONOTHER);
}
void
snmpd [6/16]: support close reason for appl_agentx_free
appl_agentx_free() closes any potential open sessions before closing the
connection and cleaning up. This function is called from multiple
contexts and the current APPL_CLOSE_REASONSHUTDOWN is not always
applicable. Add a second reason parameter that can be passed onto
appl_agentx_forceclose().
OK?
martijn@
diff --git a/application_agentx.c b/application_agentx.c
index 6a4ed49..d1efae2 100644
--- a/application_agentx.c
+++ b/application_agentx.c
@@ -77,7 +77,7 @@ struct appl_agentx_session {
void appl_agentx_listen(struct agentx_master *);
void appl_agentx_accept(int, short, void *);
-void appl_agentx_free(struct appl_agentx_connection *);
+void appl_agentx_free(struct appl_agentx_connection *, enum appl_close_reason);
void appl_agentx_recv(int, short, void *);
void appl_agentx_open(struct appl_agentx_connection *, struct ax_pdu *);
void appl_agentx_close(struct appl_agentx_session *, struct ax_pdu *);
@@ -178,7 +178,7 @@ appl_agentx_shutdown(void)
struct appl_agentx_connection *conn, *tconn;
RB_FOREACH_SAFE(conn, appl_agentx_conns, _agentx_conns, tconn)
- appl_agentx_free(conn);
+ appl_agentx_free(conn, APPL_CLOSE_REASONSHUTDOWN);
}
void
@@ -249,7 +249,8 @@ appl_agentx_backend(int fd)
}
void
-appl_agentx_free(struct appl_agentx_connection *conn)
+appl_agentx_free(struct appl_agentx_connection *conn,
+enum appl_close_reason reason)
{
struct appl_agentx_session *session;
@@ -261,7 +262,7 @@ appl_agentx_free(struct appl_agentx_connection *conn)
appl_agentx_session_free(session);
else
appl_agentx_forceclose(&(session->sess_backend),
- APPL_CLOSE_REASONSHUTDOWN);
+ reason);
}
RB_REMOVE(appl_agentx_conns, _agentx_conns, conn);
@@ -294,7 +295,8 @@ appl_agentx_recv(int fd, short event, void *cookie)
ax_free(conn->conn_ax);
conn->conn_ax = NULL;
}
- appl_agentx_free(conn);
+ appl_agentx_free(conn, errno == EPROTO ?
+ APPL_CLOSE_REASONPROTOCOLERROR : APPL_CLOSE_REASONOTHER);
return;
}
@@ -458,7 +460,7 @@ appl_agentx_recv(int fd, short event, void *cookie)
appl_agentx_forceclose(&(session->sess_backend),
APPL_CLOSE_REASONPARSEERROR);
if (TAILQ_EMPTY(&(conn->conn_sessions)))
- appl_agentx_free(conn);
+ appl_agentx_free(conn, APPL_CLOSE_REASONOTHER);
}
void
@@ -847,7 +849,7 @@ appl_agentx_send(int fd, short event, void *cookie)
log_warn("AgentX(%"PRIu32")", conn->conn_id);
ax_free(conn->conn_ax);
conn->conn_ax = NULL;
- appl_agentx_free(conn);
+ appl_agentx_free(conn, APPL_CLOSE_REASONOTHER);
return;
case 0:
return;
snmpd [5/16]: Check context existence in appl_agentx_recv
application.c checks the context where applicable, but not every
agentx-pdu goes through there (e.g. agentx-ping-pdu). Make sure
we always check the context in appl_agentx_recv()
OK?
martijn@
diff --git a/application.c b/application.c
index dd92864..a02260b 100644
--- a/application.c
+++ b/application.c
@@ -175,7 +175,7 @@ appl_shutdown(void)
}
}
-static struct appl_context *
+struct appl_context *
appl_context(const char *name, int create)
{
struct appl_context *ctx;
diff --git a/application.h b/application.h
index 8b2c567..949bbde 100644
--- a/application.h
+++ b/application.h
@@ -121,6 +121,7 @@ struct appl_backend {
void appl(void);
void appl_init(void);
void appl_shutdown(void);
+struct appl_context *appl_context(const char *, int);
enum appl_error appl_register(const char *, uint32_t, uint8_t, struct ber_oid
*,
int, int, uint8_t, uint32_t, struct appl_backend *);
enum appl_error appl_unregister(const char *, uint8_t, struct ber_oid *,
diff --git a/application_agentx.c b/application_agentx.c
index c11b666..6a4ed49 100644
--- a/application_agentx.c
+++ b/application_agentx.c
@@ -370,6 +370,12 @@ appl_agentx_recv(int fd, short event, void *cookie)
error = APPL_ERROR_PARSEERROR;
goto fail;
}
+ if (appl_context(pdu->ap_context.aos_string, 0) == 0) {
+ log_warnx("%s: %s: Unsupported context",
+ name, ax_pdutype2string(pdu->ap_header.aph_flags));
+ error = APPL_ERROR_UNSUPPORTEDCONTEXT;
+ goto fail;
+ }
}
switch (pdu->ap_header.aph_type) {
case AX_PDU_TYPE_OPEN:
snmpd [4/16]: check agentx-pdu-header flags for validity
RFC2741 section 6.1 specifies which PDUs can contain which header flags.
Check that that incoming agentx PDUs have valid flags in
appl_agentx_recv(). While here I cleaned up a few log messages some
minor restructuring to prevent the function growing too large.
OK?
martijn@
diff --git a/application_agentx.c b/application_agentx.c
index 9cc98eb..c11b666 100644
--- a/application_agentx.c
+++ b/application_agentx.c
@@ -276,13 +276,16 @@ void
appl_agentx_recv(int fd, short event, void *cookie)
{
struct appl_agentx_connection *conn = cookie;
- struct appl_agentx_session *session;
+ struct appl_agentx_session *session = NULL;
struct ax_pdu *pdu;
+ enum appl_error error;
+ char name[100];
+ snprintf(name, sizeof(name), "AgentX(%"PRIu32")", conn->conn_id);
if ((pdu = ax_recv(conn->conn_ax)) == NULL) {
if (errno == EAGAIN)
return;
- log_warn("AgentX(%"PRIu32")", conn->conn_id);
+ log_warn("%s", name);
/*
* Either the connection is dead, or we had garbage on the line.
* Both make sure we can't continue on this stream.
@@ -306,16 +309,12 @@ appl_agentx_recv(int fd, short event, void *cookie)
break;
}
if (session == NULL) {
- log_warnx("AgentX(%"PRIu32"): Session %"PRIu32" not "
- "found for request", conn->conn_id,
- pdu->ap_header.aph_sessionid);
- ax_response(conn->conn_ax, pdu->ap_header.aph_sessionid,
- pdu->ap_header.aph_transactionid,
- pdu->ap_header.aph_packetid, smi_getticks(),
- APPL_ERROR_NOTOPEN, 0, NULL, 0);
- appl_agentx_send(-1, EV_WRITE, conn);
+ log_warnx("%s: Session %"PRIu32" not found for request",
+ name, pdu->ap_header.aph_sessionid);
+ error = APPL_ERROR_NOTOPEN;
goto fail;
}
+ strlcpy(name, session->sess_backend.ab_name, sizeof(name));
/*
* RFC2741 section 7.1.1 bullet 4 is unclear on what byte order
* the response should be. My best guess is that it makes more
@@ -327,6 +326,51 @@ appl_agentx_recv(int fd, short event, void *cookie)
*/
}
+ if (pdu->ap_header.aph_flags & AX_PDU_FLAG_INSTANCE_REGISTRATION) {
+ if (pdu->ap_header.aph_type != AX_PDU_TYPE_REGISTER) {
+ log_warnx("%s: %s: Invalid INSTANCE_REGISTRATION flag",
+ name, ax_pdutype2string(pdu->ap_header.aph_flags));
+ error = APPL_ERROR_PARSEERROR;
+ goto fail;
+ }
+ }
+ if (pdu->ap_header.aph_flags & AX_PDU_FLAG_NEW_INDEX) {
+ if (pdu->ap_header.aph_type != AX_PDU_TYPE_INDEXALLOCATE &&
+ pdu->ap_header.aph_type != AX_PDU_TYPE_INDEXDEALLOCATE) {
+ log_warnx("%s: %s: Invalid NEW_INDEX flag", name,
+ ax_pdutype2string(pdu->ap_header.aph_flags));
+ error = APPL_ERROR_PARSEERROR;
+ goto fail;
+ }
+ }
+ if (pdu->ap_header.aph_flags & AX_PDU_FLAG_ANY_INDEX) {
+ if (pdu->ap_header.aph_type != AX_PDU_TYPE_INDEXALLOCATE &&
+ pdu->ap_header.aph_type != AX_PDU_TYPE_INDEXDEALLOCATE) {
+ log_warnx("%s: %s: Invalid ANY_INDEX flag", name,
+ ax_pdutype2string(pdu->ap_header.aph_flags));
+ error = APPL_ERROR_PARSEERROR;
+ goto fail;
+ }
+ }
+ if (pdu->ap_header.aph_flags & AX_PDU_FLAG_NON_DEFAULT_CONTEXT) {
+ if (pdu->ap_header.aph_type != AX_PDU_TYPE_REGISTER &&
+ pdu->ap_header.aph_type != AX_PDU_TYPE_UNREGISTER &&
+ pdu->ap_header.aph_type != AX_PDU_TYPE_ADDAGENTCAPS &&
+ pdu->ap_header.aph_type != AX_PDU_TYPE_REMOVEAGENTCAPS &&
+ pdu->ap_header.aph_type != AX_PDU_TYPE_GET &&
+ pdu->ap_header.aph_type != AX_PDU_TYPE_GETNEXT &&
+ pdu->ap_header.aph_type != AX_PDU_TYPE_GETBULK &&
+ pdu->ap_header.aph_type != AX_PDU_TYPE_INDEXALLOCATE &&
+ pdu->ap_header.aph_type != AX_PDU_TYPE_INDEXDEALLOCATE &&
+ pdu->ap_header.aph_type != AX_PDU_TYPE_NOTIFY &&
+ pdu->ap_header.aph_type != AX_PDU_TYPE_TESTSET &&
+ pdu->ap_header.aph_type != AX_PDU_TYPE_PING) {
+ log_warnx("%s: %s: Invalid NON_DEFAULT_CONTEXT flag",
+ name,
snmpd [3/16]: Distinguish between parseerror and openfailed for agentx-open-pdu
RFC2741 section 7.1.1 tells us that if a pdu can't be parsed we must
return a parseerror. Section 7.1 gives an example of "An unrecognized
value is encountered". The spec is vague is a bit vague on what
constitutes a parseerror, vs a protocol error, so I don't want to
muddle too much with that part, but let's at least return an
appropriate error when a client sends invalid data in the open request.
The spec clearly states that any error, which is not a parseerror must
be a openfailed error. So no processingerrors.
OK?
martijn@
diff --git a/application_agentx.c b/application_agentx.c
index 0d73e08..9cc98eb 100644
--- a/application_agentx.c
+++ b/application_agentx.c
@@ -411,9 +411,11 @@ appl_agentx_open(struct appl_agentx_connection *conn,
struct ax_pdu *pdu)
struct appl_agentx_session *session;
struct ber_oid oid;
char oidbuf[1024];
+ enum appl_error error = APPL_ERROR_NOERROR;
if ((session = malloc(sizeof(*session))) == NULL) {
log_warn(NULL);
+ error = APPL_ERROR_OPENFAILED;
goto fail;
}
session->sess_descr.aos_string = NULL;
@@ -432,12 +434,14 @@ appl_agentx_open(struct appl_agentx_connection *conn,
struct ax_pdu *pdu)
} else if (pdu->ap_payload.ap_open.ap_oid.aoi_idlen == 1) {
log_warnx("AgentX(%"PRIu32"): Invalid oid: Open Failed",
conn->conn_id);
+ error = APPL_ERROR_PARSEERROR;
goto fail;
}
/* RFC 2742 agentxSessionDescr */
if (pdu->ap_payload.ap_open.ap_descr.aos_slen > 255) {
log_warnx("AgentX(%"PRIu32"): Invalid descr (too long): Open "
"Failed", conn->conn_id);
+ error = APPL_ERROR_PARSEERROR;
goto fail;
}
/*
@@ -451,6 +455,7 @@ appl_agentx_open(struct appl_agentx_connection *conn,
struct ax_pdu *pdu)
pdu->ap_payload.ap_open.ap_descr.aos_string, 0) == (size_t)-1) {
log_warnx("AgentX(%"PRIu32"): Invalid descr (not UTF-8): "
"Open Failed", conn->conn_id);
+ error = APPL_ERROR_PARSEERROR;
goto fail;
}
@@ -463,6 +468,7 @@ appl_agentx_open(struct appl_agentx_connection *conn,
struct ax_pdu *pdu)
if (session->sess_descr.aos_string == NULL) {
log_warn("AgentX(%"PRIu32"): strdup: Open Failed",
conn->conn_id);
+ error = APPL_ERROR_OPENFAILED;
goto fail;
}
}
@@ -478,6 +484,7 @@ appl_agentx_open(struct appl_agentx_connection *conn,
struct ax_pdu *pdu)
conn->conn_id, session->sess_id) == -1) {
log_warn("AgentX(%"PRIu32"): asprintf: Open Failed",
conn->conn_id);
+ error = APPL_ERROR_OPENFAILED;
goto fail;
}
session->sess_backend.ab_cookie = session;
@@ -499,7 +506,7 @@ appl_agentx_open(struct appl_agentx_connection *conn,
struct ax_pdu *pdu)
return;
fail:
ax_response(conn->conn_ax, 0, pdu->ap_header.aph_transactionid,
- pdu->ap_header.aph_packetid, 0, APPL_ERROR_OPENFAILED, 0, NULL, 0);
+ pdu->ap_header.aph_packetid, 0, error, 0, NULL, 0);
appl_agentx_send(-1, EV_WRITE, conn);
if (session != NULL)
free(session->sess_descr.aos_string);
snmpd [2.2/16]: Don't set NON_DEFAULT_CONTEXT for agentx-response-pdu
> According to RFC2741 section 6.1.1 an agentx-response-pdu shouldn't have
> the NON_DEFAULT_CONTEXT set. Remove the argument from ax_response().
Here's the libagentx counterpart.
OK?
martijn@
Index: agentx.c
===
RCS file: /cvs/src/lib/libagentx/agentx.c,v
retrieving revision 1.22
diff -u -p -r1.22 agentx.c
--- agentx.c27 Dec 2022 17:10:05 - 1.22
+++ agentx.c9 Oct 2023 20:24:40 -
@@ -2733,8 +2733,7 @@ agentx_get_finalize(struct agentx_get *a
free(logmsg);
if (ax_response(ax->ax_ax, axs->axs_id, axg->axg_transactionid,
- axg->axg_packetid, AGENTX_CONTEXT_CTX(axc), 0, error, index,
- vbl, nvarbind) == -1) {
+ axg->axg_packetid, 0, error, index, vbl, nvarbind) == -1) {
agentx_log_axg_warn(axg, "Couldn't parse request");
agentx_reset(ax);
} else
@@ -4041,7 +4040,6 @@ agentx_read(struct agentx *ax)
if (ax_response(ax->ax_ax, axs->axs_id,
pdu->ap_header.aph_transactionid,
pdu->ap_header.aph_packetid,
- axc == NULL ? NULL : AGENTX_CONTEXT_CTX(axc),
0, error, 1, NULL, 0) == -1)
agentx_log_axc_warn(axc,
"transaction: %u packetid: %u: failed to send "
Index: ax.c
===
RCS file: /cvs/src/lib/libagentx/ax.c,v
retrieving revision 1.8
diff -u -p -r1.8 ax.c
--- ax.c24 Oct 2021 17:43:38 - 1.8
+++ ax.c9 Oct 2023 20:24:40 -
@@ -553,11 +553,11 @@ ax_unregister(struct ax *ax, uint32_t se
int
ax_response(struct ax *ax, uint32_t sessionid, uint32_t transactionid,
-uint32_t packetid, struct ax_ostring *context, uint32_t sysuptime,
-uint16_t error, uint16_t index, struct ax_varbind *vblist, size_t nvb)
+uint32_t packetid, uint32_t sysuptime, uint16_t error, uint16_t index,
+struct ax_varbind *vblist, size_t nvb)
{
if (ax_pdu_header(ax, AX_PDU_TYPE_RESPONSE, 0, sessionid,
- transactionid, packetid, context) == -1)
+ transactionid, packetid, NULL) == -1)
return -1;
if (ax_pdu_add_uint32(ax, sysuptime) == -1 ||
Index: ax.h
===
RCS file: /cvs/src/lib/libagentx/ax.h,v
retrieving revision 1.4
diff -u -p -r1.4 ax.h
--- ax.h2 Jan 2021 01:06:31 - 1.4
+++ ax.h9 Oct 2023 20:24:40 -
@@ -220,8 +220,7 @@ uint32_t ax_register(struct ax *, uint8_
uint32_t ax_unregister(struct ax *, uint32_t, struct ax_ostring *,
uint8_t, uint8_t, struct ax_oid *, uint32_t);
int ax_response(struct ax *, uint32_t, uint32_t, uint32_t,
-struct ax_ostring *, uint32_t, uint16_t, uint16_t,
-struct ax_varbind *, size_t);
+uint32_t, uint16_t, uint16_t, struct ax_varbind *, size_t);
void ax_pdu_free(struct ax_pdu *);
void ax_varbind_free(struct ax_varbind *);
const char *ax_error2string(enum ax_pdu_error);
Re: snmpd [1.1/16]: Don't overflow oid in agentx parser
> Currently ax.c doesn't check the maximum length of an OID ax_pdutooid. > This can lead to a buffer overflow. Even though it must be fixed, I > don't think there's a big risk here, since an attacker would need to have > access to the agentx socket, which by default is disabled and defaults > to root:_agentx when enabled. Here's the libagentx counterpart. OK? martijn@ Index: ax.c === RCS file: /cvs/src/lib/libagentx/ax.c,v retrieving revision 1.8 diff -u -p -r1.8 ax.c --- ax.c24 Oct 2021 17:43:38 - 1.8 +++ ax.c9 Oct 2023 20:14:13 - @@ -1262,6 +1262,8 @@ ax_pdutooid(struct ax_pdu_header *header } buf++; oid->aoi_include = *buf; + if (oid->aoi_idlen > AX_OID_MAX_LEN) + goto fail; for (buf += 2; i < oid->aoi_idlen; i++, buf += 4) oid->aoi_id[i] = ax_pdutoh32(header, buf);
snmpd [2.1/16]: Don't set NON_DEFAULT_CONTEXT for agentx-response-pdu
According to RFC2741 section 6.1.1 an agentx-response-pdu shouldn't have
the NON_DEFAULT_CONTEXT set. Remove the argument from ax_response().
OK?
martijn@
diff --git a/application_agentx.c b/application_agentx.c
index c7a2a26..0d73e08 100644
--- a/application_agentx.c
+++ b/application_agentx.c
@@ -311,8 +311,7 @@ appl_agentx_recv(int fd, short event, void *cookie)
pdu->ap_header.aph_sessionid);
ax_response(conn->conn_ax, pdu->ap_header.aph_sessionid,
pdu->ap_header.aph_transactionid,
- pdu->ap_header.aph_packetid,
- &(pdu->ap_context), smi_getticks(),
+ pdu->ap_header.aph_packetid, smi_getticks(),
APPL_ERROR_NOTOPEN, 0, NULL, 0);
appl_agentx_send(-1, EV_WRITE, conn);
goto fail;
@@ -370,8 +369,8 @@ appl_agentx_recv(int fd, short event, void *cookie)
case AX_PDU_TYPE_PING:
ax_response(conn->conn_ax, pdu->ap_header.aph_sessionid,
pdu->ap_header.aph_transactionid,
- pdu->ap_header.aph_packetid, &(pdu->ap_context),
- smi_getticks(), APPL_ERROR_NOERROR, 0, NULL, 0);
+ pdu->ap_header.aph_packetid, smi_getticks(),
+ APPL_ERROR_NOERROR, 0, NULL, 0);
appl_agentx_send(-1, EV_WRITE, conn);
break;
case AX_PDU_TYPE_INDEXALLOCATE:
@@ -380,8 +379,8 @@ appl_agentx_recv(int fd, short event, void *cookie)
ax_pdutype2string(pdu->ap_header.aph_type));
ax_response(conn->conn_ax, pdu->ap_header.aph_sessionid,
pdu->ap_header.aph_transactionid,
- pdu->ap_header.aph_packetid, &(pdu->ap_context),
- smi_getticks(), APPL_ERROR_PROCESSINGERROR, 1,
+ pdu->ap_header.aph_packetid, smi_getticks(),
+ APPL_ERROR_PROCESSINGERROR, 1,
pdu->ap_payload.ap_vbl.ap_varbind,
pdu->ap_payload.ap_vbl.ap_nvarbind);
appl_agentx_send(-1, EV_WRITE, conn);
@@ -392,8 +391,8 @@ appl_agentx_recv(int fd, short event, void *cookie)
ax_pdutype2string(pdu->ap_header.aph_type));
ax_response(conn->conn_ax, pdu->ap_header.aph_sessionid,
pdu->ap_header.aph_transactionid,
- pdu->ap_header.aph_packetid, &(pdu->ap_context),
- smi_getticks(), APPL_ERROR_PROCESSINGERROR, 1,
+ pdu->ap_header.aph_packetid, smi_getticks(),
+ APPL_ERROR_PROCESSINGERROR, 1,
NULL, 0);
appl_agentx_send(-1, EV_WRITE, conn);
break;
@@ -492,16 +491,15 @@ appl_agentx_open(struct appl_agentx_connection *conn,
struct ax_pdu *pdu)
log_info("%s: %s %s: Open", session->sess_backend.ab_name, oidbuf,
session->sess_descr.aos_string);
- ax_response(conn->conn_ax, session->sess_id,
pdu->ap_header.aph_transactionid,
- pdu->ap_header.aph_packetid, NULL, smi_getticks(),
APPL_ERROR_NOERROR, 0,
- NULL, 0);
+ ax_response(conn->conn_ax, session->sess_id,
+ pdu->ap_header.aph_transactionid, pdu->ap_header.aph_packetid,
+ smi_getticks(), APPL_ERROR_NOERROR, 0, NULL, 0);
appl_agentx_send(-1, EV_WRITE, conn);
return;
fail:
ax_response(conn->conn_ax, 0, pdu->ap_header.aph_transactionid,
- pdu->ap_header.aph_packetid, NULL, 0, APPL_ERROR_OPENFAILED, 0,
- NULL, 0);
+ pdu->ap_header.aph_packetid, 0, APPL_ERROR_OPENFAILED, 0, NULL, 0);
appl_agentx_send(-1, EV_WRITE, conn);
if (session != NULL)
free(session->sess_descr.aos_string);
@@ -521,7 +519,7 @@ appl_agentx_close(struct appl_agentx_session *session,
struct ax_pdu *pdu)
ax_response(conn->conn_ax, pdu->ap_header.aph_sessionid,
pdu->ap_header.aph_transactionid, pdu->ap_header.aph_packetid,
- &(pdu->ap_context), smi_getticks(), APPL_ERROR_NOERROR, 0, NULL, 0);
+ smi_getticks(), APPL_ERROR_NOERROR, 0, NULL, 0);
appl_agentx_send(-1, EV_WRITE, conn);
}
@@ -593,7 +591,7 @@ appl_agentx_register(struct appl_agentx_session *session,
struct ax_pdu *pdu)
fail:
ax_response(session->sess_conn->conn_ax, session->sess_id,
pdu->ap_header.aph_transactionid, pdu->ap_header.aph_packetid,
- &(pdu->ap_context), smi_getticks(), error, 0, NULL, 0);
+ smi_getticks(), error, 0, NULL, 0);
appl_agentx_send(-1, EV_WRITE, session->sess_conn);
}
@@ -620,7 +618,7 @@ appl_agentx_unregister(struct appl_agentx_session *session,
struct ax_pdu *pdu)
fail:
ax_response(session->sess_conn->conn_ax, session->sess_id,
pdu->ap_header.aph_transactionid,
snmpd [1.1/16]: Don't overflow oid in agentx parser
Currently ax.c doesn't check the maximum length of an OID ax_pdutooid. This can lead to a buffer overflow. Even though it must be fixed, I don't think there's a big risk here, since an attacker would need to have access to the agentx socket, which by default is disabled and defaults to root:_agentx when enabled. OK? martijn@ diff --git a/ax.c b/ax.c index 63add68..27580a6 100644 --- a/ax.c +++ b/ax.c @@ -1442,6 +1442,8 @@ ax_pdutooid(struct ax_pdu_header *header, struct ax_oid *oid, } buf++; oid->aoi_include = *buf; + if (oid->aoi_idlen > AX_OID_MAX_LEN) + goto fail; for (buf += 2; i < oid->aoi_idlen; i++, buf += 4) oid->aoi_id[i] = ax_pdutoh32(header, buf);
Prevent off-by-one accounting hang in out-of-swap situations
Diff below makes out-of-swap checks more robust.
When a system is low on swap, two variables are used to adapt the
behavior of the page daemon and fault handler:
`swpginuse'
indicates how much swap space is being currently used
`swpgonly'
indicates how much swap space stores content of pages that are no
longer living in memory.
The diff below changes the heuristic to detect if the system is currently
out-of-swap. In my tests it makes the system more stable by preventing
hangs. It prevents from hangs that occur when the system has more than 99%
of it swap space filled. When this happen, the checks using the variables
above to figure out if we are out-of-swap might never be true because of:
- Races between the fault-handler and the accounting of the page daemon
due to asynchronous swapping
- The swap partition being never completely allocated (bad pages,
off-by-one, rounding error, size of a swap cluster...)
- Possible off-by-one accounting errors in swap space
So I'm adapting uvm_swapisfull() to return true as soon as more than
99% of the swap space is filled with pages which are no longer in memory.
I also introduce uvm_swapisfilled() to prevent later failures if there is
less than a cluster of space available (the minimum we try to swap out).
This prevent deadlocking if a few slots (pg_flags & PQ_SWAPBACKED) &&
- uvmexp.swpginuse == uvmexp.swpages) {
+ if ((p->pg_flags & PQ_SWAPBACKED) && uvm_swapisfilled())
uvmpd_dropswap(p);
- }
/*
* the page we are looking at is dirty. we must
@@ -917,9 +914,7 @@ uvmpd_scan(struct uvm_pmalloc *pma, struct
uvm_constraint_range *constraint)
*/
free = uvmexp.free - BUFPAGES_DEFICIT;
swap_shortage = 0;
- if (free < uvmexp.freetarg &&
- uvmexp.swpginuse == uvmexp.swpages &&
- !uvm_swapisfull() &&
+ if (free < uvmexp.freetarg && uvm_swapisfilled() && !uvm_swapisfull() &&
pages_freed == 0) {
swap_shortage = uvmexp.freetarg - free;
}
diff --git sys/uvm/uvm_swap.c sys/uvm/uvm_swap.c
index 27963259eba..913b2366a7c 100644
--- sys/uvm/uvm_swap.c
+++ sys/uvm/uvm_swap.c
@@ -1516,8 +1516,30 @@ ReTry: /* XXXMRG */
}
/*
- * uvm_swapisfull: return true if all of available swap is allocated
- * and in use.
+ * uvm_swapisfilled: return true if the amount of free space in swap is
+ * smaller than the size of a cluster.
+ *
+ * As long as some swap slots are being used by pages currently in memory,
+ * it is possible to reuse them. Even if the swap space has been completly
+ * filled we do not consider it full.
+ */
+int
+uvm_swapisfilled(void)
+{
+ int result;
+
+ mtx_enter(_swap_data_lock);
+ KASSERT(uvmexp.swpginuse <= uvmexp.swpages);
+ result = (uvmexp.swpginuse + SWCLUSTPAGES) >= uvmexp.swpages;
+ mtx_leave(_swap_data_lock);
+
+ return result;
+}
+
+/*
+ * uvm_swapisfull: return true if the amount of pages only in swap
+ * accounts for more than 99% of the total swap space.
+ *
*/
int
uvm_swapisfull(void)
@@ -1526,7 +1548,7 @@ uvm_swapisfull(void)
mtx_enter(_swap_data_lock);
KASSERT(uvmexp.swpgonly <= uvmexp.swpages);
- result = (uvmexp.swpgonly == uvmexp.swpages);
+ result = (uvmexp.swpgonly >= (uvmexp.swpages * 99 / 100));
mtx_leave(_swap_data_lock);
return result;
diff --git sys/uvm/uvm_swap.h sys/uvm/uvm_swap.h
index 9904fe58cd7..f60237405ca 100644
--- sys/uvm/uvm_swap.h
+++ sys/uvm/uvm_swap.h
@@ -42,6 +42,7 @@ int uvm_swap_put(int, struct vm_page **,
int, int);
intuvm_swap_alloc(int *, boolean_t);
void uvm_swap_free(int, int);
void uvm_swap_markbad(int, int);
+intuvm_swapisfilled(void);
intuvm_swapisfull(void);
void uvm_swap_freepages(struct vm_page **, int);
#ifdef HIBERNATE
diff --git sys/uvm/uvmexp.h sys/uvm/uvmexp.h
index de5f5fa367c..144494b73ff 100644
--- sys/uvm/uvmexp.h
+++ sys/uvm/uvmexp.h
@@ -83,7 +83,7 @@ struct uvmexp {
/* swap */
int nswapdev; /* [S] number of configured swap devices in system */
int swpages;/* [S] number of PAGE_SIZE'ed swap pages */
- int swpginuse; /* [K] number of swap pages in use */
+ int swpginuse; /* [S] number of swap pages in use */
int swpgonly; /* [a] number of swap pages in use, not also in RAM */
int nswget; /* [a] number of swap pages moved from disk to RAM */
int nanon; /* XXX number total of anon's in system */
Re: small fix in uvmpd_scan_inactive()
On Tue, Oct 17, 2023 at 12:41:57PM +0200, Martin Pieuchot wrote:
> Diff below merges two equivalent if blocks. No functional change, ok?
ok tb
>
>
> Index: uvm/uvm_pdaemon.c
> ===
> RCS file: /cvs/src/sys/uvm/uvm_pdaemon.c,v
> retrieving revision 1.107
> diff -u -p -r1.107 uvm_pdaemon.c
> --- uvm/uvm_pdaemon.c 16 Oct 2023 11:32:54 - 1.107
> +++ uvm/uvm_pdaemon.c 17 Oct 2023 10:28:25 -
> @@ -650,6 +650,11 @@ uvmpd_scan_inactive(struct uvm_pmalloc *
> p->offset >> PAGE_SHIFT,
> swslot + swcpages);
> swcpages++;
> + rw_exit(slock);
> +
> + /* cluster not full yet? */
> + if (swcpages < swnpages)
> + continue;
> }
> } else {
> /* if p == NULL we must be doing a last swap i/o */
> @@ -666,14 +671,6 @@ uvmpd_scan_inactive(struct uvm_pmalloc *
>* for object pages, we always do the pageout.
>*/
> if (swap_backed) {
> - if (p) {/* if we just added a page to cluster */
> - rw_exit(slock);
> -
> - /* cluster not full yet? */
> - if (swcpages < swnpages)
> - continue;
> - }
> -
> /* starting I/O now... set up for it */
> npages = swcpages;
> ppsp = swpps;
>
small fix in uvmpd_scan_inactive()
Diff below merges two equivalent if blocks. No functional change, ok?
Index: uvm/uvm_pdaemon.c
===
RCS file: /cvs/src/sys/uvm/uvm_pdaemon.c,v
retrieving revision 1.107
diff -u -p -r1.107 uvm_pdaemon.c
--- uvm/uvm_pdaemon.c 16 Oct 2023 11:32:54 - 1.107
+++ uvm/uvm_pdaemon.c 17 Oct 2023 10:28:25 -
@@ -650,6 +650,11 @@ uvmpd_scan_inactive(struct uvm_pmalloc *
p->offset >> PAGE_SHIFT,
swslot + swcpages);
swcpages++;
+ rw_exit(slock);
+
+ /* cluster not full yet? */
+ if (swcpages < swnpages)
+ continue;
}
} else {
/* if p == NULL we must be doing a last swap i/o */
@@ -666,14 +671,6 @@ uvmpd_scan_inactive(struct uvm_pmalloc *
* for object pages, we always do the pageout.
*/
if (swap_backed) {
- if (p) {/* if we just added a page to cluster */
- rw_exit(slock);
-
- /* cluster not full yet? */
- if (swcpages < swnpages)
- continue;
- }
-
/* starting I/O now... set up for it */
npages = swcpages;
ppsp = swpps;
Re: nfsd: don't clear SB_NOINTR flag
On Mon, Oct 16, 2023 at 10:17:50PM +0300, Vitaliy Makkoveev wrote:
> This socket comes from userland, so this flag is never set. This makes
> SB_NOINTR flag immutable, because we only set this bit on NFS client
> socket buffers for all it's lifetime.
>
> I want to do this because this flag modifies sblock() behaviour and it's
> not clean which lock should protect it for standalone sblock().
>
> ok?
>
> I want to completely remove SB_NOINTR flag. Only NFS client sets it, but
> since the socket never passed to userland, this flag is useless, because
> we can't send singnal to kernel thread. So for this sblock()/sbwait()
> sleep is uninterruptible. But I want to not mix NFS server and NFS
> client diffs.
>
> Index: sys/nfs/nfs_syscalls.c
> ===
> RCS file: /cvs/src/sys/nfs/nfs_syscalls.c,v
> retrieving revision 1.119
> diff -u -p -r1.119 nfs_syscalls.c
> --- sys/nfs/nfs_syscalls.c3 Aug 2023 09:49:09 - 1.119
> +++ sys/nfs/nfs_syscalls.c16 Oct 2023 19:00:02 -
> @@ -276,9 +276,7 @@ nfssvc_addsock(struct file *fp, struct m
> m_freem(m);
> }
> solock(so);
> - so->so_rcv.sb_flags &= ~SB_NOINTR;
> so->so_rcv.sb_timeo_nsecs = INFSLP;
> - so->so_snd.sb_flags &= ~SB_NOINTR;
> so->so_snd.sb_timeo_nsecs = INFSLP;
> sounlock(so);
> if (tslp)
>
To make the possible review easier, please look at sbin/nfsd/nfsd.c. We
have two cases, for UDP case we just pass newly created socket to kernel
side nfsd, for TCP case we pass only accepted sockets:
104 main(int argc, char *argv[])
105 {
/* ... */
208 if (udpflag) {
209 if ((sock = socket(AF_INET, SOCK_DGRAM, 0)) == -1) {
210 syslog(LOG_ERR, "can't create udp socket");
211 return (1);
212 }
/* ... */
228 nfsdargs.sock = sock;
229 nfsdargs.name = NULL;
230 nfsdargs.namelen = 0;
231 if (nfssvc(NFSSVC_ADDSOCK, ) == -1) {
232 syslog(LOG_ERR, "can't Add UDP socket");
233 return (1);
234 }
235 (void)close(sock);
236 }
/* ... */
241 if (tcpflag) {
242 if ((tcpsock = socket(AF_INET, SOCK_STREAM, 0)) == -1) {
243 syslog(LOG_ERR, "can't create tcp socket");
244 return (1);
245 }
/* ... */
259 if (listen(tcpsock, 5) == -1) {
260 syslog(LOG_ERR, "listen failed");
261 return (1);
262 }
/* ... */
269 }
/* ... */
276 /*
277 * Loop forever accepting connections and passing the sockets
278 * into the kernel for the mounts.
279 */
280 for (;;) {
/* ... */
298 if (tcpflag) {
299 len = sizeof(inetpeer);
300 if ((msgsock = accept(tcpsock,
301 (struct sockaddr *), )) == -1) {
/* ... */
313 nfsdargs.sock = msgsock;
314 nfsdargs.name = (caddr_t)
315 nfsdargs.namelen = sizeof(inetpeer);
316 if (nfssvc(NFSSVC_ADDSOCK, ) == -1) {
317 syslog(LOG_ERR, "can't Add TCP socket");
318 return (1);
319 }
320 (void)close(msgsock);
321 }
322 }
For the NFSSVC_ADDSOCK command, sys_nfssvc() gets associated descriptor
and passes it to nfssvc_addsock().
141 sys_nfssvc(struct proc *p, void *v, register_t *retval)
142 {
/* ... */
171 switch (flags) {
172 case NFSSVC_ADDSOCK:
173 error = copyin(SCARG(uap, argp), , sizeof(nfsdarg));
174 if (error)
175 return (error);
176
177 error = getsock(p, nfsdarg.sock, );
/* ... */
194 error = nfssvc_addsock(fp, nam);
nfssvc_addsock() gets associated socket, does necessary setup and passes
it to nfsd. There is the only place where we clean this flag.
225 nfssvc_addsock(struct file *fp, struct mbuf *mynam)
226 {
/* ... */
278 solock(so);
279 so->so_rcv.sb_flags &= ~SB_NOINTR;
280 so->so_rcv.sb_timeo_nsecs = INFSLP;
281 so->so_snd.sb_flags &= ~SB_NOINTR;
282 so->so_snd.sb_timeo_nsecs = INFSLP;
283 sounlock(so);
The only place where we set this flag is the NFS client related socket.
It always created in kernel space and it never comes to the userland.
This socket can't be passed to nfsd.
234 nfs_connect(struct nfsmount *nmp, struct nfsreq *rep)
235 {
/* ... */
247 error = socreate(saddr->sa_family, >nm_so,
Re: Improve IPv6 link-local support in bgpd
Claudio Jeker(cje...@diehard.n-r-g.com) on 2023.10.16 09:23:12 +0200:
> This diff fixes a few more things when establishing connections with
> link-local IPv6 addresses. In get_alternate_addr() the interface scope
> of the connection is recovered and then passed to the RDE. The RDE can
> then use this scope id to insert link-local addresses with the correct
> scope.
looks good.
> I built a regress test for this which passes with this diff.
> Now probably more is needed because IPv6 link-local addresses are a gift
> that keep on giving. One thing to implement on top of this is template
> matching for link local -- which allows to auto-configure sessions more
> easily. This will probably follow soon.
it would be nice if that could match just on interfaces. As in "neighbor
", with some syntactic sugar so its clear this is for
link-local?
> --
> :wq Claudio
>
> Index: bgpd.h
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.h,v
> retrieving revision 1.477
> diff -u -p -r1.477 bgpd.h
> --- bgpd.h30 Aug 2023 08:16:28 - 1.477
> +++ bgpd.h9 Oct 2023 13:50:36 -
> @@ -796,6 +796,7 @@ struct session_up {
> struct bgpd_addrremote_addr;
> struct capabilities capa;
> uint32_tremote_bgpid;
> + unsigned intif_scope;
> uint16_tshort_as;
> };
>
> @@ -1439,6 +1440,7 @@ void kr_ifinfo(char *);
> void kr_net_reload(u_int, uint64_t, struct network_head *);
> int kr_reload(void);
> int get_mpe_config(const char *, u_int *, u_int *);
> +uint8_t mask2prefixlen(sa_family_t, struct sockaddr *);
>
> /* log.c */
> void log_peer_info(const struct peer_config *, const char *, ...)
> Index: kroute.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
> retrieving revision 1.305
> diff -u -p -r1.305 kroute.c
> --- kroute.c 1 Jun 2023 09:47:34 - 1.305
> +++ kroute.c 9 Oct 2023 13:54:25 -
> @@ -168,8 +168,6 @@ struct kroute6*kroute6_match(struct kta
> void kroute_detach_nexthop(struct ktable *, struct knexthop *);
>
> uint8_t prefixlen_classful(in_addr_t);
> -uint8_t mask2prefixlen(in_addr_t);
> -uint8_t mask2prefixlen6(struct sockaddr_in6 *);
> uint64_t ift2ifm(uint8_t);
> const char *get_media_descr(uint64_t);
> const char *get_linkstate(uint8_t, int);
> @@ -2419,21 +2417,28 @@ prefixlen_classful(in_addr_t ina)
> return (8);
> }
>
> -uint8_t
> -mask2prefixlen(in_addr_t ina)
> +static uint8_t
> +mask2prefixlen4(struct sockaddr_in *sa_in)
> {
> + in_addr_t ina;
> +
> + if (sa_in->sin_len == 0)
> + return (0);
> + ina = sa_in->sin_addr.s_addr;
> if (ina == 0)
> return (0);
> else
> return (33 - ffs(ntohl(ina)));
> }
>
> -uint8_t
> +static uint8_t
> mask2prefixlen6(struct sockaddr_in6 *sa_in6)
> {
> uint8_t *ap, *ep;
> u_intl = 0;
>
> + if (sa_in6->sin6_len == 0)
> + return (0);
> /*
>* sin6_len is the size of the sockaddr so subtract the offset of
>* the possibly truncated sin6_addr struct.
> @@ -2480,6 +2485,19 @@ mask2prefixlen6(struct sockaddr_in6 *sa_
> return (l);
> }
>
> +uint8_t
> +mask2prefixlen(sa_family_t af, struct sockaddr *mask)
> +{
> + switch (af) {
> + case AF_INET:
> + return mask2prefixlen4((struct sockaddr_in *)mask);
> + case AF_INET6:
> + return mask2prefixlen6((struct sockaddr_in6 *)mask);
> + default:
> + fatalx("%s: unsupported af", __func__);
> + }
> +}
> +
> const struct if_status_description
> if_status_descriptions[] = LINK_STATE_DESCRIPTIONS;
> const struct ifmedia_description
> @@ -3079,9 +3097,7 @@ dispatch_rtmsg_addr(struct rt_msghdr *rt
> case AF_INET:
> sa_in = (struct sockaddr_in *)rti_info[RTAX_NETMASK];
> if (sa_in != NULL) {
> - if (sa_in->sin_len != 0)
> - kf->prefixlen =
> - mask2prefixlen(sa_in->sin_addr.s_addr);
> + kf->prefixlen = mask2prefixlen4(sa_in);
> } else if (rtm->rtm_flags & RTF_HOST)
> kf->prefixlen = 32;
> else
> @@ -3091,8 +3107,7 @@ dispatch_rtmsg_addr(struct rt_msghdr *rt
> case AF_INET6:
> sa_in6 = (struct sockaddr_in6 *)rti_info[RTAX_NETMASK];
> if (sa_in6 != NULL) {
> - if (sa_in6->sin6_len != 0)
> - kf->prefixlen = mask2prefixlen6(sa_in6);
> + kf->prefixlen = mask2prefixlen6(sa_in6);
> } else if (rtm->rtm_flags & RTF_HOST)
> kf->prefixlen = 128;
>
Re: log.c use buffered IO
Theo Buehler(t...@theobuehler.org) on 2023.10.17 09:13:15 +0200:
> On Mon, Oct 16, 2023 at 12:19:17PM +0200, Claudio Jeker wrote:
> > I dislike how log.c does all these asprintf() calls with dubious
> > workaround calls in case asprintf() fails.
>
> You're not alone.
>
> > IMO it is easier to use the stdio provided buffers and fflush() to get
> > "atomic" writes on stderr. At least from my understanding this is the
> > reason to go all this lenght to do a single fprintf call.
>
> This makes sense, but I don't know the history here.
as far as i can remember, it was done so it would still be able to work
somewhat when out of memeory.
> > We need this since in privsep daemons can log from multiple processes
> > concurrently and we want the log to be not too mangled.
> >
> > While there also use one static buffer to handle log_warn() and vfatalc()
> > where the error message is first expanded and then printed with
> > logit(LOG_ERR, "%s: %s", fmtbuf, strerror(saved_errno));
> >
> > Any opinions?
>
> I like this much better than the existing code.
>
> ok tb, one small request inline
>
> > --
> > :wq Claudio
> >
> > Index: log.c
> > ===
> > RCS file: /cvs/src/usr.sbin/bgpd/log.c,v
> > retrieving revision 1.64
> > diff -u -p -r1.64 log.c
> > --- log.c 21 Mar 2017 12:06:55 - 1.64
> > +++ log.c 16 Oct 2023 09:42:55 -
> > @@ -29,6 +29,7 @@
> > static int debug;
> > static int verbose;
> > static const char *log_procname;
> > +static char logbuf[4096], fmtbuf[4096];
> >
> > void
> > log_init(int n_debug, int facility)
> > @@ -41,6 +42,8 @@ log_init(int n_debug, int facility)
> >
> > if (!debug)
> > openlog(__progname, LOG_PID | LOG_NDELAY, facility);
> > + else
> > + setvbuf(stderr, logbuf, _IOFBF, sizeof(logbuf));
can you turn this around (if (debug) ...)
> > tzset();
> > }
> > @@ -77,18 +80,11 @@ logit(int pri, const char *fmt, ...)
> > void
> > vlog(int pri, const char *fmt, va_list ap)
> > {
> > - char*nfmt;
> > int saved_errno = errno;
> >
> > if (debug) {
>
> Could log_init() and vlog() either both use if (debug) or both if (!debug),
> please? Missing the ! in log_init() confused me for a few minutes since it
> looked wrong.
yes please :)
>
> > - /* best effort in out of mem situations */
> > - if (asprintf(, "%s\n", fmt) == -1) {
> > - vfprintf(stderr, fmt, ap);
> > - fprintf(stderr, "\n");
> > - } else {
> > - vfprintf(stderr, nfmt, ap);
> > - free(nfmt);
> > - }
> > + vfprintf(stderr, fmt, ap);
> > + fprintf(stderr, "\n");
> > fflush(stderr);
> > } else
> > vsyslog(pri, fmt, ap);
> > @@ -99,7 +95,6 @@ vlog(int pri, const char *fmt, va_list a
> > void
> > log_warn(const char *emsg, ...)
> > {
> > - char*nfmt;
> > va_list ap;
> > int saved_errno = errno;
> >
> > @@ -108,16 +103,8 @@ log_warn(const char *emsg, ...)
> > logit(LOG_ERR, "%s", strerror(saved_errno));
> > else {
> > va_start(ap, emsg);
> > -
> > - if (asprintf(, "%s: %s", emsg,
> > - strerror(saved_errno)) == -1) {
> > - /* we tried it... */
> > - vlog(LOG_ERR, emsg, ap);
> > - logit(LOG_ERR, "%s", strerror(saved_errno));
> > - } else {
> > - vlog(LOG_ERR, nfmt, ap);
> > - free(nfmt);
> > - }
> > + (void)vsnprintf(fmtbuf, sizeof(fmtbuf), emsg, ap);
> > + logit(LOG_ERR, "%s: %s", fmtbuf, strerror(saved_errno));
> > va_end(ap);
> > }
> >
> > @@ -159,21 +146,20 @@ log_debug(const char *emsg, ...)
> > static void
> > vfatalc(int code, const char *emsg, va_list ap)
> > {
> > - static char s[BUFSIZ];
> > const char *sep;
> >
> > if (emsg != NULL) {
> > - (void)vsnprintf(s, sizeof(s), emsg, ap);
> > + (void)vsnprintf(fmtbuf, sizeof(fmtbuf), emsg, ap);
> > sep = ": ";
> > } else {
> > - s[0] = '\0';
> > + fmtbuf[0] = '\0';
> > sep = "";
> > }
> > if (code)
> > logit(LOG_CRIT, "fatal in %s: %s%s%s",
> > - log_procname, s, sep, strerror(code));
> > + log_procname, fmtbuf, sep, strerror(code));
> > else
> > - logit(LOG_CRIT, "fatal in %s%s%s", log_procname, sep, s);
> > + logit(LOG_CRIT, "fatal in %s%s%s", log_procname, sep, fmtbuf);
> > }
> >
> > void
> >
>
Re: log.c use buffered IO
On Mon, Oct 16, 2023 at 12:19:17PM +0200, Claudio Jeker wrote:
> I dislike how log.c does all these asprintf() calls with dubious
> workaround calls in case asprintf() fails.
You're not alone.
> IMO it is easier to use the stdio provided buffers and fflush() to get
> "atomic" writes on stderr. At least from my understanding this is the
> reason to go all this lenght to do a single fprintf call.
This makes sense, but I don't know the history here.
> We need this since in privsep daemons can log from multiple processes
> concurrently and we want the log to be not too mangled.
>
> While there also use one static buffer to handle log_warn() and vfatalc()
> where the error message is first expanded and then printed with
> logit(LOG_ERR, "%s: %s", fmtbuf, strerror(saved_errno));
>
> Any opinions?
I like this much better than the existing code.
ok tb, one small request inline
> --
> :wq Claudio
>
> Index: log.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/log.c,v
> retrieving revision 1.64
> diff -u -p -r1.64 log.c
> --- log.c 21 Mar 2017 12:06:55 - 1.64
> +++ log.c 16 Oct 2023 09:42:55 -
> @@ -29,6 +29,7 @@
> static intdebug;
> static intverbose;
> static const char*log_procname;
> +static char logbuf[4096], fmtbuf[4096];
>
> void
> log_init(int n_debug, int facility)
> @@ -41,6 +42,8 @@ log_init(int n_debug, int facility)
>
> if (!debug)
> openlog(__progname, LOG_PID | LOG_NDELAY, facility);
> + else
> + setvbuf(stderr, logbuf, _IOFBF, sizeof(logbuf));
>
> tzset();
> }
> @@ -77,18 +80,11 @@ logit(int pri, const char *fmt, ...)
> void
> vlog(int pri, const char *fmt, va_list ap)
> {
> - char*nfmt;
> int saved_errno = errno;
>
> if (debug) {
Could log_init() and vlog() either both use if (debug) or both if (!debug),
please? Missing the ! in log_init() confused me for a few minutes since it
looked wrong.
> - /* best effort in out of mem situations */
> - if (asprintf(, "%s\n", fmt) == -1) {
> - vfprintf(stderr, fmt, ap);
> - fprintf(stderr, "\n");
> - } else {
> - vfprintf(stderr, nfmt, ap);
> - free(nfmt);
> - }
> + vfprintf(stderr, fmt, ap);
> + fprintf(stderr, "\n");
> fflush(stderr);
> } else
> vsyslog(pri, fmt, ap);
> @@ -99,7 +95,6 @@ vlog(int pri, const char *fmt, va_list a
> void
> log_warn(const char *emsg, ...)
> {
> - char*nfmt;
> va_list ap;
> int saved_errno = errno;
>
> @@ -108,16 +103,8 @@ log_warn(const char *emsg, ...)
> logit(LOG_ERR, "%s", strerror(saved_errno));
> else {
> va_start(ap, emsg);
> -
> - if (asprintf(, "%s: %s", emsg,
> - strerror(saved_errno)) == -1) {
> - /* we tried it... */
> - vlog(LOG_ERR, emsg, ap);
> - logit(LOG_ERR, "%s", strerror(saved_errno));
> - } else {
> - vlog(LOG_ERR, nfmt, ap);
> - free(nfmt);
> - }
> + (void)vsnprintf(fmtbuf, sizeof(fmtbuf), emsg, ap);
> + logit(LOG_ERR, "%s: %s", fmtbuf, strerror(saved_errno));
> va_end(ap);
> }
>
> @@ -159,21 +146,20 @@ log_debug(const char *emsg, ...)
> static void
> vfatalc(int code, const char *emsg, va_list ap)
> {
> - static char s[BUFSIZ];
> const char *sep;
>
> if (emsg != NULL) {
> - (void)vsnprintf(s, sizeof(s), emsg, ap);
> + (void)vsnprintf(fmtbuf, sizeof(fmtbuf), emsg, ap);
> sep = ": ";
> } else {
> - s[0] = '\0';
> + fmtbuf[0] = '\0';
> sep = "";
> }
> if (code)
> logit(LOG_CRIT, "fatal in %s: %s%s%s",
> - log_procname, s, sep, strerror(code));
> + log_procname, fmtbuf, sep, strerror(code));
> else
> - logit(LOG_CRIT, "fatal in %s%s%s", log_procname, sep, s);
> + logit(LOG_CRIT, "fatal in %s%s%s", log_procname, sep, fmtbuf);
> }
>
> void
>
