| Message ID | 20210114133910.282686-1-plautrba@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libselinux: Always close status page fd | expand |
On Thu, Jan 14, 2021 at 7:42 AM Petr Lautrbach <plautrba@redhat.com> 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@redhat.com> > --- > 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; > } > > @@ -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); > } > -- > 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.
On Thu, Jan 14, 2021 at 8:30 AM William Roberts <bill.c.roberts@gmail.com> wrote: > > On Thu, Jan 14, 2021 at 7:42 AM Petr Lautrbach <plautrba@redhat.com> 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@redhat.com> > > --- > > 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; > > } > > > > @@ -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); > > } > > -- > > 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. Sorry I misread that, I missed the assignment from fd to that variable... Ack, this should fix that umount issue hopefully I didn't test that though.
On Thu, Jan 14, 2021 at 8:33 AM William Roberts <bill.c.roberts@gmail.com> wrote: > > On Thu, Jan 14, 2021 at 8:30 AM William Roberts > <bill.c.roberts@gmail.com> wrote: > > > > On Thu, Jan 14, 2021 at 7:42 AM Petr Lautrbach <plautrba@redhat.com> 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@redhat.com> > > > --- > > > 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; > > > } > > > > > > @@ -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); > > > } > > > -- > > > 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. > > Sorry I misread that, I missed the assignment from fd to that variable... > > Ack, this should fix that umount issue hopefully I didn't test that though. As an aside though, I did notice a bug in the existing code: A failure in: selinux_status_fd = avc_netlink_acquire_fd(); Will cause the code to still return success to the caller of selinux_status_open(). And cause the close to return EBADF which is silently ignored and doesn't matter.
William Roberts <bill.c.roberts@gmail.com> writes: > On Thu, Jan 14, 2021 at 7:42 AM Petr Lautrbach <plautrba@redhat.com> 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@redhat.com> >> --- >> 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() > >> @@ -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.
On Thu, Jan 14, 2021 at 8:54 AM Petr Lautrbach <plautrba@redhat.com> wrote: > > William Roberts <bill.c.roberts@gmail.com> writes: > > > On Thu, Jan 14, 2021 at 7:42 AM Petr Lautrbach <plautrba@redhat.com> 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@redhat.com> > >> --- > >> 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. >
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; } @@ -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); }
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@redhat.com> --- libselinux/src/sestatus.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)