Page MenuHomeFreeBSD

Properly set vimage contexts in sys/rpc
ClosedPublic

Authored by delphij on Aug 17 2015, 6:01 AM.
Tags
None
Referenced Files
Unknown Object (File)
Nov 20 2024, 12:15 AM
Unknown Object (File)
Nov 4 2024, 6:19 PM
Unknown Object (File)
Oct 14 2024, 7:56 PM
Unknown Object (File)
Oct 2 2024, 9:08 PM
Unknown Object (File)
Oct 2 2024, 9:35 AM
Unknown Object (File)
Oct 2 2024, 8:17 AM
Unknown Object (File)
Oct 2 2024, 4:53 AM
Unknown Object (File)
Oct 2 2024, 1:23 AM
Subscribers

Details

Summary

There are quite a few places where vimage context is not being properly set.
We believe this relates to FreeNAS #11012.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 129
Build 129: arc lint + arc unit

Event Timeline

delphij retitled this revision from to Properly set vimage contexts in sys/rpc.
delphij updated this object.
delphij edited the test plan for this revision. (Show Details)
delphij added a reviewer: bz.
melifaro added a reviewer: melifaro.
melifaro added a subscriber: melifaro.

The changes itself seems to be fine.

However, I'm not quite sure on how could we get a crash in sa6_recoverscope() due to misplaced VNET pointer:
in6_sockaddr() allocates new memory block, properly zeroed. We can get some garbage in both address/scopeid field, but it should not crash.
ifnet_byindex() should simply return NULL in worst case.

Btw, we could probably consider using some sort of VNET_ macro to simplify all similar calls

#define SO_VOID_VNET_METHOD(so, _method, ...)                           \
     CURVNET_SET((so)->so_vnet);                                     \
     (*(so)->so_proto->pr_usrreqs->_method)(__VA_ARGS__);            \
     CURVNET_RESTORE();

#define SO_VNET_METHOD(so, _method, _ret, ...)                          \
     CURVNET_SET((so)->so_vnet);                                     \
     _ret = (*(so)->so_proto->pr_usrreqs->_method)(__VA_ARGS__);     \
     CURVNET_RESTORE();
This revision is now accepted and ready to land.Aug 18 2015, 11:18 AM

I would prefer if we would not start conflating so/pr/pru callouts with the curvnet things.
It'll make grep-ing a lot harder and sounds more like we'll introduce more bugs and also current recursions we don't want.

However, I'm not quite sure on how could we get a crash in sa6_recoverscope() due to misplaced VNET pointer:
in6_sockaddr() allocates new memory block, properly zeroed. We can get some garbage in both address/scopeid field, but it should not crash.
ifnet_byindex() should simply return NULL in worst case.

Well, the thread's vnet pointer could be NULL and it's that pointer gets deferenced.

This revision was automatically updated to reflect the committed changes.

The cause of the panic is that sa6_recoverscope() uses V_if_index in a sanity checking when the address is link-local. Probably the reporter has used an LLA for NFS over IPv6.

I think setting curvnet around in6_sockaddr() to make it possible to be called from a thread with no default vnet is more reasonable than doing around every pru_sockaddr() call, because guaranteeing pru_xxx methods be robust prevents similar bugs from being introduced again in the future. How about the patch at http://people.allbsd.org/~hrs/FreeBSD/in6_sockaddr_vnet.20150820-1.diff ?