rtnetlink: Check for allocation failure in nlattr_get_multipath()
ClosedPublic
Actions

Authored by pouria on Mon, May 11, 9:23 PM.

Details

Reviewers

markj
glebius
melifaro

Group Reviewers

network

Commits

rGc9b5ce2422cc: rtnetlink: Check for allocation failure in nlattr_get_multipath()
rG3e2308d57080: rtnetlink: Check for allocation failure in nlattr_get_multipath()
rG188631e43a1a: rtnetlink: Check for allocation failure in nlattr_get_multipath()

Summary

Check for alloction failure on npt_alloc() for RTA_MULTIPATH
attributes in nlattr_get_multipath().
Also, add tests for maximum number of rtnexthop in rtnetlink.

Reported by: Joshua Rogers of AISLE Research Team
MFC after: 3 days

Test Plan

Send more than 22 RTA_MULTIPATH in a single request.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 72996
Build 69879: arc lint + arc unit

Event Timeline

pouria created this revision.Mon, May 11, 9:23 PM

Herald added a subscriber: imp. · View Herald TranscriptMon, May 11, 9:23 PM

pouria requested review of this revision.Mon, May 11, 9:23 PM

Harbormaster completed remote builds in B72996: Diff 177610.Mon, May 11, 9:23 PM

pouria edited the summary of this revision. (Show Details)Mon, May 11, 9:26 PM

pouria added inline comments.Tue, May 12, 9:33 AM

sys/netlink/route/rt.c
475	I'm fine with ENOMEM and ENOBUFS too.

Use ENOMEM instead.
it's absolutely not normal to create a route with more than 22 nhops in a single request.
But as @markj suggested, I decided to use ENOMEM instead.
At least, it more matches the description in errno.2

Harbormaster completed remote builds in B73006: Diff 177626.Tue, May 12, 10:13 AM

zlei added a subscriber: zlei.Tue, May 12, 10:14 AM

zlei added inline comments.

sys/netlink/route/rt.c
475	Actually `npt_alloc()` is not allocating memory, so I think the usage of `E2BIG` is a proper here.

pouria added inline comments.Tue, May 12, 10:27 AM

sys/netlink/route/rt.c
475	That was my initial thought too. But the description in errno says: The number of bytes used for the argument and environment list of the new process exceeded the current limit (NCARGS in <sys/param.h>). That part changed my mind. Do you still think the E2BIG is more appropriate?

pouria retitled this revision from routing: Check for allocation failure in nlattr_get_multipath() to rtnetlink: Check for allocation failure in nlattr_get_multipath().Tue, May 12, 11:32 AM

pouria mentioned this in D56964: rtnetlink: Check for allocation failure in create_nexthop_from_attrs().Tue, May 12, 12:29 PM

pouria mentioned this in D56965: rtnetlink: Check for allocation failure in _nl_store_ifp_cookie().Tue, May 12, 12:40 PM

markj accepted this revision.Tue, May 12, 4:08 PM

markj added inline comments.

tests/sys/netlink/test_rtnl_route.c
16	Why is this a local include?
364	Do you need to do this in a cleanup handler?

This revision is now accepted and ready to land.Tue, May 12, 4:08 PM

pouria added inline comments.Tue, May 12, 9:12 PM

tests/sys/netlink/test_rtnl_route.c
16	I don't remember the reason anymore :( It compiles fine now, but AFAICR it wasn't fine without local include when I initially wrote this test. We use the same local includes in other C tests under netlink (e.g., test_snl.c, test_snl_generic.c). I'll fix it in another revision.
364	It doesn't work under atf cleanup. I can't pass stack variables to another function from within atf cleanup. I had to inline the cleanup_route_by_dst() function for each test, which is uglier IMHO.