Properly set vimage contexts in sys/rpc
ClosedPublic
Actions

Authored by delphij on Aug 17 2015, 6:01 AM.

Details

Reviewers

melifaro
hrs
glebius
bz

Group Reviewers

network

Commits

rS286894: Set curvnet context inside the RPC code in more places.

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 updated this revision to Diff 7998.Aug 17 2015, 6:01 AM

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.

Herald added a subscriber: imp. · View Herald TranscriptAug 17 2015, 6:01 AM

delphij added reviewers: hrs, glebius, network.Aug 17 2015, 6:03 AM

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.

In D3398#69610, @melifaro wrote:

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.

Closed by commit rS286894: Set curvnet context inside the RPC code in more places. (authored by delphij). · Explain WhyAug 18 2015, 6:13 PM

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 ?