Page MenuHomeFreeBSD

bsnmpd(1): optimize interface description processing
ClosedPublic

Authored by eugen_grosbein.net on Jul 26 2018, 3:22 PM.

Details

Summary
  • teach bsnmpd to allocate memory dynamically for interface descriptions

to decrease memory usage for common case and not to break if long description occurs;

  • skip calling strlen() for each description and each SNMP request for MIB-II/ifXTable's ifAlias.
Test Plan
  1. Do:

ifconfig ng0 descr "70-chars long description"
ifconfig ng1 descr "70-chars long description"

and observe bsnmpd spamming logs with messages:

SIOCGIFDESCR (ng0): too long (70)
SIOCGIFDESCR (ng1): too long (70)

And empty ifAlias'es in the ifXTable instead of descriptions.

  1. Apply the patch, rebuild and restart bsnmpd and see no errors and correctly filles ifAlias values.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

avg added a comment.Jul 26 2018, 3:38 PM

Some comments inline, but in general looks good to me.
Thanks!

contrib/bsnmp/snmp_mibII/mibII.c
448 ↗(On Diff #45878)

I would order this static variable before other variables and then group the new non-static variables with the existing variables.

449 ↗(On Diff #45878)

Is nitmes available to userland? If yes, then it would be better to use it.

539 ↗(On Diff #45878)

I guess this should have been kmib[0] > 0 ?

760 ↗(On Diff #45878)

Missing space after if.
Also, it would be more canonical to explicitly compare with NULL.

eugen_grosbein.net marked 4 inline comments as done.Jul 26 2018, 4:09 PM
eugen_grosbein.net added inline comments.
contrib/bsnmp/snmp_mibII/mibII.c
448 ↗(On Diff #45878)

Ok

449 ↗(On Diff #45878)

Yes, it is available.

539 ↗(On Diff #45878)

The case kmib[0] < 0 is processed above, so if (kmib[0]) is, indeed, if (kmib[0] > 0) but shorter as later variant pushes length of this line to its limits. Anyway, I change this.

760 ↗(On Diff #45878)

Ok

eugen_grosbein.net marked 4 inline comments as done.

Style changes.

avg accepted this revision.Jul 26 2018, 4:29 PM
This revision is now accepted and ready to land.Jul 26 2018, 4:29 PM
bz added a subscriber: bz.Jul 26 2018, 6:31 PM

Hmm RFC 2863 says:

ifAlias OBJECT-TYPE

SYNTAX      DisplayString (SIZE(0..64))

So where is our real problem?

bz requested changes to this revision.Jul 26 2018, 6:32 PM

I guess I want the real problem to be identified and understood (or a good description for this change and why) rather than patching something that simply masks a different problem.

This revision now requires changes to proceed.Jul 26 2018, 6:32 PM
In D16459#349396, @bz wrote:

Hmm RFC 2863 says:
ifAlias OBJECT-TYPE

SYNTAX      DisplayString (SIZE(0..64))

Nice catch. Does it mean that complying SNMP application must support at most 64 (or 65) octects lenght ifAlias or it must support at least so? Note that Cisco supports "at least" or more: http://cfn.cloudapps.cisco.com/ITDIT/CFN/Dispatch?act=featdesc&task=display&featureId=742

So where is our real problem?

"Test plan" section describes our problems with bsnmpd:

  • bsnmpd spams logs indefinitly if a system has interface with description over 65 characters;
  • returned ifAlias is inaccurately empty for such an interface;
  • every system interface increases size of bsnmpd data set for next char[65] array no matter if it is needed or not, when an interface has no description at all;
  • every SNMP request for ifXTable/ifAlias OID results in ineffective strlen() call for mentioned array.

Proposed patch fixed all these problems.

bz added a comment.Jul 26 2018, 7:45 PM
In D16459#349396, @bz wrote:

Hmm RFC 2863 says:
ifAlias OBJECT-TYPE

SYNTAX      DisplayString (SIZE(0..64))

Nice catch. Does it mean that complying SNMP application must support at most 64 (or 65) octects lenght ifAlias or it must support at least so? Note that Cisco supports "at least" or more: http://cfn.cloudapps.cisco.com/ITDIT/CFN/Dispatch?act=featdesc&task=display&featureId=742

So where is our real problem?

"Test plan" section describes our problems with bsnmpd:

  • bsnmpd spams logs indefinitly if a system has interface with description over 65 characters;

Yes but that might not be bsnmpd's fault (well it might with the "spamming") but should the system allow them to be set longer? Should they be silently truncated when exporting (or being read by bsnmpd)? Just masking that problem away is what I mean doesn't exist. What I am saying is that if this patch is considered to be what "we want" then we should document the possible violation of the RFC (hopefully only if the user changes a sysctl elsewhere, or if not under which circumstances) somewhere (ideally in a man page). But someone needs to make a decision on why the base system behaves the way it does, whether that is the right thing, or if not where we'll "break" the standard.

In D16459#349416, @bz wrote:

Yes but that might not be bsnmpd's fault (well it might with the "spamming")

That is definitely bsnmp's problem.

but should the system allow them to be set longer?

The kernel has no SNMP support. But it has interface descriptions limited by sysctl net.ifdescr_maxlen == 1024 by default.
bsnmpd obtains descriptions using sysctl(3) interface and short 65-octet buffer and fails.

Should they be silently truncated when exporting (or being read by bsnmpd)? Just masking that problem away is what I mean doesn't exist.

Why do you calling it "masking"? Have you actually read the patch? It solves the problem, not masks it.

What I am saying is that if this patch is considered to be what "we want" then we should document the possible violation of the RFC (hopefully only if the user changes a sysctl elsewhere, or if not under which circumstances) somewhere (ideally in a man page).

Why would be that "violation" of the RFC? I could not find a prohibition to support longer ifAlias anywhere in RFC, and Cisco supports longer values too. And patched bsnmpd won't emit longer ifAlias'es unless system administrator or software configures an interface with long description.

avg removed a reviewer: avg.Jul 27 2018, 5:23 AM
avg added a subscriber: avg.Jul 27 2018, 5:31 AM

Well, the standard says (0 ..64) and that means from zero to 64.
Also, my impression is that, according to the RFC, ifAlias is something that should be settable over SNMP and it should be persistent.

I found this interesting link on the topic and I must admit that I know very little about it:
http://irobert.info/display/IRINFO/Understanding+ifDescr%2C+ifName+and+ifAlias

harti added a comment.Jul 27 2018, 5:59 AM
In D16459#349483, @avg wrote:

Well, the standard says (0 ..64) and that means from zero to 64.
Also, my impression is that, according to the RFC, ifAlias is something that should be settable over SNMP and it should be persistent.
I found this interesting link on the topic and I must admit that I know very little about it:
http://irobert.info/display/IRINFO/Understanding+ifDescr%2C+ifName+and+ifAlias

I wish I had this reference when writing the code in the first place :-).

I looked up RFC2578. Section 9 (Refined Syntax) says for OCTET STRING:

(3) the size in octets of the value may be refined by raising the

lower-bounds, by reducing the upper-bounds, and/or by reducing the
alternative size choices.

Below that:

No other types of refinements can be specified in the SYNTAX clause.

I think the point here is to allow dumb clients with static buffers to handle all legal cases.

I would recommend to just silently clip the description at the maximum size.

In D16459#349483, @avg wrote:

Well, the standard says (0 ..64) and that means from zero to 64.
Also, my impression is that, according to the RFC, ifAlias is something that should be settable over SNMP and it should be persistent.

bsnmpd does not support SNMP SET for ifAlias currently

The only change comparing with previous revision is added truncation of interface description obtained with sysctl(3) if it appears longer than 64 octets.

I'm going to commit latest revision soon unless an objection is raised.

avg added a comment.Jul 31 2018, 7:15 AM

I'm going to commit latest revision soon unless an objection is raised.

You would still need a mentor approval.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 18 2018, 10:59 AM
This revision was automatically updated to reflect the committed changes.