Re: snmpd(8): Better determining searchrange end
On Tue, Aug 30, 2022 at 04:31:20PM +0200, Martijn van Duren wrote:
> Doing overlapping regions is hard...
>
> At application.c:1341 we currently assign region->ar_oid to oid.
> However, with overlapping regions this can cause a recursion, because
> region might be the parent of the previous region that would cause the
> oid to traverse back and cause a loop.
>
> This can easily be fixed by changing this line to vb->av_oid, but I also
> found that the code doesn't produce the cleanest searchrange end,
> adjacent regions might not be taken into account on a single run because
> of the else statement on line 1363.
>
> With the following registration it will result in:
> blocklist: Registering 1.3.6.1.4.1.30155.1.9.129 context() priority(1)
> timeout(1.50s)
> AgentX(556403008/718113859): Registering 1.3.6.1.4.1.30155.1 context()
> priority(1) timeout(5.00s)
> AgentX(556403008/718113859): Registering 1.3.6.1.4.1.30155.2 context()
> priority(1) timeout(5.00s)
> AgentX(556403008/718113859): Registering 1.3.6.1.4.1.30155.5 context()
> priority(1) timeout(5.00s)
> AgentX(556403008/718113859): Registering 1.3.6.1.4.1.30155.6 context()
> priority(1) timeout(5.00s)
> ...
> GetNextRequest{333882979, 0, 0, {{1.3.6.1.4.1.30155.1.9.128.1.24.3:null}}}
> AgentX(556403008/718113859): GetNextRequest{1517163039, 0, 0,
> {{1.3.6.1.4.1.30155.1.9.128.1.24.3-1.3.6.1.4.1.30155.1.9.129:null}}}
> AgentX(556403008/718113859): Response{1517163039, 0, 0,
> {{1.3.6.1.4.1.30155.1.9.128.1.24.3:endOfMibView}}}
> blocklist: GetNextRequest{1339241491, 0, 0,
> {{1.3.6.1.4.1.30155.1.9.129(incl)-1.3.6.1.4.1.30155.1.9.130:null}}}
> blocklist: Response{1339241491, 0, 0,
> {{1.3.6.1.4.1.30155.1.9.129:endOfMibView}}}
> AgentX(556403008/718113859): GetNextRequest{1545296030, 0, 0,
> {{1.3.6.1.4.1.30155.1.9.130(incl)-1.3.6.1.4.1.30155.2:null}}}
> AgentX(556403008/718113859): Response{1545296030, 0, 0,
> {{1.3.6.1.4.1.30155.1.10.1.0:2}}}
>
> to
> ...
> GetNextRequest{2078129483, 0, 0,
> {{1.3.6.1.4.1.30155.1.9.129.1.1.1.192.168.153.0.24:null}}}
> blocklist: GetNextRequest{-1870096078, 0, 0,
> {{1.3.6.1.4.1.30155.1.9.129.1.1.1.192.168.153.0.24-1.3.6.1.4.1.30155.1.9.130:null}}}
> blocklist: Response{-1870096078, 0, 0,
> {{1.3.6.1.4.1.30155.1.9.129.1.1.1.192.168.153.0.24:endOfMibView}}}
> AgentX(3175611302/2074212055): GetNextRequest{-867062125, 0, 0,
> {{1.3.6.1.4.1.30155.1.9.130(incl)-1.3.6.1.4.1.30155.3:null}}}
> AgentX(3175611302/2074212055): Response{-867062125, 0, 0,
> {{1.3.6.1.4.1.30155.1.10.1.0:2}}}
>
> OK?
looks good
ok tb
>
> martijn@
>
> Index: application.c
> ===
> RCS file: /cvs/src/usr.sbin/snmpd/application.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 application.c
> --- application.c 29 Aug 2022 18:05:08 - 1.12
> +++ application.c 30 Aug 2022 14:29:50 -
> @@ -1287,7 +1287,7 @@ appl_varbind_backend(struct appl_varbind
> struct appl_request_upstream *ureq = ivb->avi_request_upstream;
> struct appl_region search, *region, *pregion;
> struct appl_varbind *vb = &(ivb->avi_varbind);
> - struct ber_oid oid;
> + struct ber_oid oid, nextsibling;
> int next, cmp;
>
> next = ureq->aru_pdu->be_type == SNMP_C_GETNEXTREQ ||
> @@ -1338,33 +1338,41 @@ appl_varbind_backend(struct appl_varbind
> }
> ivb->avi_region = region;
> if (next) {
> - oid = region->ar_oid;
> + oid = vb->av_oid;
> + /*
> + * For the searchrange end we only want contiguous regions.
> + * This means directly connecting, or overlapping with the same
> + * backend.
> + */
> do {
> pregion = region;
> region = appl_region_next(ureq->aru_ctx, , pregion);
> - if (region != NULL &&
> - appl_region_cmp(region, pregion) > 0)
> - oid = region->ar_oid;
> - else
> + if (region == NULL) {
> + oid = pregion->ar_oid;
> ober_oid_nextsibling();
> - } while (region != NULL &&
> - region->ar_backend == pregion->ar_backend);
> - if (region == NULL) {
> - vb->av_oid_end = pregion->ar_oid;
> - if (pregion->ar_instance &&
> - vb->av_oid_end.bo_n < BER_MAX_OID_LEN)
> - vb->av_oid_end.bo_id[vb->av_oid_end.bo_n++] = 0;
> - else
> - ober_oid_nextsibling(&(vb->av_oid_end));
> - } else {
> - if (ober_oid_cmp(&(region->ar_oid),
> - &(ivb->avi_region->ar_oid)) == 2)
> - vb->av_oid_end = region->ar_oid;
> - else {
> -
snmpd(8): Better determining searchrange end
Doing overlapping regions is hard...
At application.c:1341 we currently assign region->ar_oid to oid.
However, with overlapping regions this can cause a recursion, because
region might be the parent of the previous region that would cause the
oid to traverse back and cause a loop.
This can easily be fixed by changing this line to vb->av_oid, but I also
found that the code doesn't produce the cleanest searchrange end,
adjacent regions might not be taken into account on a single run because
of the else statement on line 1363.
With the following registration it will result in:
blocklist: Registering 1.3.6.1.4.1.30155.1.9.129 context() priority(1)
timeout(1.50s)
AgentX(556403008/718113859): Registering 1.3.6.1.4.1.30155.1 context()
priority(1) timeout(5.00s)
AgentX(556403008/718113859): Registering 1.3.6.1.4.1.30155.2 context()
priority(1) timeout(5.00s)
AgentX(556403008/718113859): Registering 1.3.6.1.4.1.30155.5 context()
priority(1) timeout(5.00s)
AgentX(556403008/718113859): Registering 1.3.6.1.4.1.30155.6 context()
priority(1) timeout(5.00s)
...
GetNextRequest{333882979, 0, 0, {{1.3.6.1.4.1.30155.1.9.128.1.24.3:null}}}
AgentX(556403008/718113859): GetNextRequest{1517163039, 0, 0,
{{1.3.6.1.4.1.30155.1.9.128.1.24.3-1.3.6.1.4.1.30155.1.9.129:null}}}
AgentX(556403008/718113859): Response{1517163039, 0, 0,
{{1.3.6.1.4.1.30155.1.9.128.1.24.3:endOfMibView}}}
blocklist: GetNextRequest{1339241491, 0, 0,
{{1.3.6.1.4.1.30155.1.9.129(incl)-1.3.6.1.4.1.30155.1.9.130:null}}}
blocklist: Response{1339241491, 0, 0,
{{1.3.6.1.4.1.30155.1.9.129:endOfMibView}}}
AgentX(556403008/718113859): GetNextRequest{1545296030, 0, 0,
{{1.3.6.1.4.1.30155.1.9.130(incl)-1.3.6.1.4.1.30155.2:null}}}
AgentX(556403008/718113859): Response{1545296030, 0, 0,
{{1.3.6.1.4.1.30155.1.10.1.0:2}}}
to
...
GetNextRequest{2078129483, 0, 0,
{{1.3.6.1.4.1.30155.1.9.129.1.1.1.192.168.153.0.24:null}}}
blocklist: GetNextRequest{-1870096078, 0, 0,
{{1.3.6.1.4.1.30155.1.9.129.1.1.1.192.168.153.0.24-1.3.6.1.4.1.30155.1.9.130:null}}}
blocklist: Response{-1870096078, 0, 0,
{{1.3.6.1.4.1.30155.1.9.129.1.1.1.192.168.153.0.24:endOfMibView}}}
AgentX(3175611302/2074212055): GetNextRequest{-867062125, 0, 0,
{{1.3.6.1.4.1.30155.1.9.130(incl)-1.3.6.1.4.1.30155.3:null}}}
AgentX(3175611302/2074212055): Response{-867062125, 0, 0,
{{1.3.6.1.4.1.30155.1.10.1.0:2}}}
OK?
martijn@
Index: application.c
===
RCS file: /cvs/src/usr.sbin/snmpd/application.c,v
retrieving revision 1.12
diff -u -p -r1.12 application.c
--- application.c 29 Aug 2022 18:05:08 - 1.12
+++ application.c 30 Aug 2022 14:29:50 -
@@ -1287,7 +1287,7 @@ appl_varbind_backend(struct appl_varbind
struct appl_request_upstream *ureq = ivb->avi_request_upstream;
struct appl_region search, *region, *pregion;
struct appl_varbind *vb = &(ivb->avi_varbind);
- struct ber_oid oid;
+ struct ber_oid oid, nextsibling;
int next, cmp;
next = ureq->aru_pdu->be_type == SNMP_C_GETNEXTREQ ||
@@ -1338,33 +1338,41 @@ appl_varbind_backend(struct appl_varbind
}
ivb->avi_region = region;
if (next) {
- oid = region->ar_oid;
+ oid = vb->av_oid;
+ /*
+* For the searchrange end we only want contiguous regions.
+* This means directly connecting, or overlapping with the same
+* backend.
+*/
do {
pregion = region;
region = appl_region_next(ureq->aru_ctx, , pregion);
- if (region != NULL &&
- appl_region_cmp(region, pregion) > 0)
- oid = region->ar_oid;
- else
+ if (region == NULL) {
+ oid = pregion->ar_oid;
ober_oid_nextsibling();
- } while (region != NULL &&
- region->ar_backend == pregion->ar_backend);
- if (region == NULL) {
- vb->av_oid_end = pregion->ar_oid;
- if (pregion->ar_instance &&
- vb->av_oid_end.bo_n < BER_MAX_OID_LEN)
- vb->av_oid_end.bo_id[vb->av_oid_end.bo_n++] = 0;
- else
- ober_oid_nextsibling(&(vb->av_oid_end));
- } else {
- if (ober_oid_cmp(&(region->ar_oid),
- &(ivb->avi_region->ar_oid)) == 2)
- vb->av_oid_end = region->ar_oid;
- else {
- vb->av_oid_end = ivb->avi_region->ar_oid;
- ober_oid_nextsibling(&(vb->av_oid_end));
+ break;
}
- }
+
