Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore early ZONE_ATTR_ values #1149

Merged
merged 1 commit into from Apr 7, 2022
Merged

Conversation

citrus-it
Copy link
Member

While testing the s10 brand under the upcoming r151042 release, a service failed to come online:

# /usr/sbin/ipsecalgs -s
ipsecalgs: Unable to determine zone IP type: Invalid argument

zone_getattr(10, ZONE_ATTR_FLAGS, 0x08047DE8, 2) Err#22 EINVAL

Some dtrace showed that the branded zone was requesting attribute number 14:

r151042# dtrace -n 'zone_getattr:entry/arg0==11/{self->t++; trace(arg1)}' -n 'zone_getattr:return/self->t/{self->t--; trace(arg1)}' -n 'zone_status_get:return/self->t/{trace(arg1)}' -n 'zone_list_access:return/self->t/{trace(arg1)}' -n 's10_getattr:entry/self->t/{trace(arg1)}' -n 's10_getattr:return/self->t/{trace(arg1)}'

CPU     ID                    FUNCTION:NAME
  2  28085               zone_getattr:entry             32768
  2  37731           zone_status_get:return                 4
  2  28078          zone_list_access:return                 1
  2   9380                s10_getattr:entry             32768
  2   9381               s10_getattr:return                 0
  2  28086              zone_getattr:return                 1
  2  28085               zone_getattr:entry                14
  2  37731           zone_status_get:return                 4
  2  28078          zone_list_access:return                 1
  2  28086              zone_getattr:return                22

and, unfortunately, our attribute 14 is not what it once was:

s10# grep ZONE_ATTR_FLAGS /usr/include/sys/zone.h
#define ZONE_ATTR_FLAGS         14
r151042# grep ZONE_ATTR_FLAGS /usr/include/sys/zone.h
#define   ZONE_ATTR_FLAGS            13

it was renumbered as part of 4ece279

This restores the numbering of the early flags, so that they are what branded zones expect. It also re-synchronises the ones in the 20-29 range with the values in gate and moves the OmniOS/SmartOS-only ones to 30+

@citrus-it citrus-it requested a review from danmcd April 6, 2022 15:04
oetiker
oetiker previously approved these changes Apr 6, 2022
@citrus-it
Copy link
Member Author

With this patch, things are much happier in the zone and the ipsecalgs service is online.

s10# truss -t zone_getattr /usr/sbin/ipsecalgs -s
zone_lookup("")                                 = 1
zone_getattr(1, 32768, 0xFEFFDA88, 1)           = 1
zone_lookup("")                                 = 1
zone_getattr(1, ZONE_ATTR_FLAGS, 0x08047DB0, 2) = 2

Retrieval of the ZONE_ATTR_FLAGS attribute is succeeding.

@citrus-it citrus-it marked this pull request as ready for review April 6, 2022 15:36
@citrus-it citrus-it requested a review from hadfl as a code owner April 6, 2022 15:36
Copy link
Member

@danmcd danmcd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nobody outside the platform (illumos and any extras in a distro) consume these values, right? I don't think it is, but it's a question that should linger.

@citrus-it
Copy link
Member Author

citrus-it commented Apr 6, 2022

Nobody outside the platform (illumos and any extras in a distro) consume these values, right? I don't think it is, but it's a question that should linger.

I can't imagine that they do, certainly not the higher-numbered ones which are related to the RSS capping.
A search on github for ZONE_ATTR_FLAGS shows almost entirely illumos/opensolaris/unleashed stuff.

There's at least one project on there (glibc on illumos) which has the ZONE_FLAGS_ATTR value hard coded to 14, which is the value we're restoring it to.

danmcd
danmcd previously approved these changes Apr 6, 2022
Copy link
Member

@danmcd danmcd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So glibc is doing something stupid, huh? Why am I NOT surprised. :)

Copy link
Member

@danmcd danmcd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the truss(1) ones.

@hadfl hadfl merged commit efbed64 into omniosorg:master Apr 7, 2022
@citrus-it citrus-it deleted the zoneattr branch April 7, 2022 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants