On Sun, Aug 23, 2026 at 7:52 AM Oleg Nesterov <[email protected]> wrote: > > On 08/23, Bill Wendling wrote: > > > > The compiler attribute __counted_by_ptr associates a pointer field of a > > struct with a sibling field within the same struct that specifies the > > element count of the allocated memory. This enables KASAN and fortified > > bounds-checking to detect out-of-bounds accesses to the pointer field at > > runtime. > > > > We can add the __counted_by_ptr attribute to the 'forward' and 'reverse' > > pointer fields of 'struct uid_gid_map', which are counted by > > 'nr_extents'. Since 'nr_extents' is defined in a sibling anonymous > > struct inside an anonymous union, the nearest common non-anonymous > > struct level is 'struct uid_gid_map' itself, which is supported by the > > compiler. > > > > However, doing so has runtime implications. In the original > > implementation of insert_extent(), elements are written to > > map->forward[map->nr_extents] before map->nr_extents is incremented: > > > > if (map->nr_extents < UID_GID_MAP_MAX_BASE_EXTENTS) > > dest = &map->extent[map->nr_extents]; > > else > > dest = &map->forward[map->nr_extents]; > > > > *dest = *extent; > > map->nr_extents++; > > > > At the time of writing to 'map->forward[map->nr_extents]', > > map->nr_extents is still 5, but we are accessing index 5 (which is the > > 6th element). Under __counted_by_ptr(nr_extents), the compiler and > > KASAN expect the accessed index to be strictly less than > > map->nr_extents. Therefore, accessing index 5 when the count is 5 > > triggers an out-of-bounds panic/trap at runtime. > > > > To resolve this, insert_extent() is refactored to increment > > map->nr_extents first, and then use map->nr_extents - 1 as the index: > > > > map->nr_extents++; > > if (map->nr_extents <= UID_GID_MAP_MAX_BASE_EXTENTS) > > dest = &map->extent[map->nr_extents - 1]; > > else > > dest = &map->forward[map->nr_extents - 1]; > > > > *dest = *extent; > > I leave this to you and other reviewers (add Alexey), you can safely > ignore my nit. > > To me > > if (map->nr_extents < UID_GID_MAP_MAX_BASE_EXTENTS) > dest = &map->extent; > else > dest = &map->forward; > > map->nr_extents++; > dest[map->nr_extents - 1] = *extent; > > looks a bit more clear, but this is minor/subjective. > > Either way, I think this needs a short comment to explain why do we > need to increment ->nr_extents first, then subtract 1. IOW, to explain > why (say) > > dest[map->nr_extents++] = *extent; > > would be wrong. > Hi Oleg,
I added a comment before the code in question explaining why it's organized that way. I certainly don't hate your version, but for simplicity of reviews I just left the code as is. -bw > Oleg. > > > Assisted-by: Gemini Next > > Signed-off-by: Bill Wendling <[email protected]> > > --- > > Cc: Kees Cook <[email protected]> > > Cc: "Gustavo A. R. Silva" <[email protected]> > > Cc: Christian Brauner <[email protected]> > > Cc: Aleksa Sarai <[email protected]> > > Cc: Jan Kara <[email protected]> > > Cc: Nathan Chancellor <[email protected]> > > Cc: Miguel Ojeda <[email protected]> > > Cc: Thomas Gleixner <[email protected]> > > Cc: Nicolas Schier <[email protected]> > > Cc: Gary Guo <[email protected]> > > Cc: "Thomas Weißschuh" <[email protected]> > > Cc: Alice Ryhl <[email protected]> > > Cc: Douglas Anderson <[email protected]> > > Cc: Anand Moon <[email protected]> > > Cc: Oleg Nesterov <[email protected]> > > Cc: [email protected] > > Cc: [email protected] > > Cc: [email protected] > > --- > > include/linux/user_namespace.h | 4 ++-- > > kernel/user_namespace.c | 8 ++++---- > > 2 files changed, 6 insertions(+), 6 deletions(-) > > > > diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h > > index e38d9e60569f..2962256eddf7 100644 > > --- a/include/linux/user_namespace.h > > +++ b/include/linux/user_namespace.h > > @@ -29,8 +29,8 @@ struct uid_gid_map { /* 64 bytes -- 1 cache line */ > > u32 nr_extents; > > }; > > struct { > > - struct uid_gid_extent *forward; > > - struct uid_gid_extent *reverse; > > + struct uid_gid_extent *forward > > __counted_by_ptr(nr_extents); > > + struct uid_gid_extent *reverse > > __counted_by_ptr(nr_extents); > > }; > > }; > > }; > > diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c > > index 0bed462e9b2a..7e5371d8f515 100644 > > --- a/kernel/user_namespace.c > > +++ b/kernel/user_namespace.c > > @@ -809,13 +809,13 @@ static int insert_extent(struct uid_gid_map *map, > > struct uid_gid_extent *extent) > > map->reverse = NULL; > > } > > > > - if (map->nr_extents < UID_GID_MAP_MAX_BASE_EXTENTS) > > - dest = &map->extent[map->nr_extents]; > > + map->nr_extents++; > > + if (map->nr_extents <= UID_GID_MAP_MAX_BASE_EXTENTS) > > + dest = &map->extent[map->nr_extents - 1]; > > else > > - dest = &map->forward[map->nr_extents]; > > + dest = &map->forward[map->nr_extents - 1]; > > > > *dest = *extent; > > - map->nr_extents++; > > return 0; > > } > > > > -- > > 2.55.0.860.g4b6b3295ed-goog > > >
