On Thu, Jan 14, 2021 at 8:54 AM Petr Lautrbach <plautrba@xxxxxxxxxx> wrote:
>
> William Roberts <bill.c.roberts@xxxxxxxxx> writes:
>
> > On Thu, Jan 14, 2021 at 7:42 AM Petr Lautrbach <plautrba@xxxxxxxxxx> wrote:
> >>
> >> According to mmap(2) after the mmap() call has returned, the file
> >> descriptor, fd, can be closed immediately without invalidating the
> >> mapping.
> >>
> >> Signed-off-by: Petr Lautrbach <plautrba@xxxxxxxxxx>
> >> ---
> >> libselinux/src/sestatus.c | 6 ++----
> >> 1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/libselinux/src/sestatus.c b/libselinux/src/sestatus.c
> >> index 9ff2785d876a..6a243b7bcdfb 100644
> >> --- a/libselinux/src/sestatus.c
> >> +++ b/libselinux/src/sestatus.c
> >> @@ -298,11 +298,10 @@ int selinux_status_open(int fallback)
> >> goto error;
> >>
> >> selinux_status = mmap(NULL, pagesize, PROT_READ, MAP_SHARED, fd, 0);
> >> + close(fd);
> >> if (selinux_status == MAP_FAILED) {
> >> - close(fd);
> >> goto error;
> >> }
> >> - selinux_status_fd = fd;
> >> last_seqno = (uint32_t)(-1);
> >>
> >> /* sequence must not be changed during references */
> >> @@ -379,6 +378,7 @@ void selinux_status_close(void)
> >> avc_netlink_release_fd();
> >> avc_netlink_close();
> >> selinux_status = NULL;
> >> + close(selinux_status_fd);
> >> return;
> >> }
>
> I'll drop this one. It's already closed by avc_netlink_close()
We can actually get rid of selinux_status_fd all together. The call to
avc_netlink_acquire_fd() just returns it's static cache of the fd acquired
in avc_netlink_open(). So we can just pair the avc_netlink_open with
it's avc_netlink_code().
>
> >
> >> @@ -388,7 +388,5 @@ void selinux_status_close(void)
> >> munmap(selinux_status, pagesize);
> >> selinux_status = NULL;
> >>
> >> - close(selinux_status_fd);
> >> - selinux_status_fd = -1;
> >> last_seqno = (uint32_t)(-1);
>
> I believe this is correct. selinux_stats_fd is not assigned when mmap()
> doesn't return MAP_FAILED
>
> >> }
> >> --
> >> 2.30.0
> >>
> >
> > Nack, the fd in the mmap of the status page and the selinux_status_fd
> > (avc mount) are different fd's.
> > The selinux_status_fd is for the AVC netlink socket fallback. If you
> > drop those hunks I'd take the patch.
>
