* [PATCH] libselinux: Always close status page fd
@ 2021-01-14 13:39 Petr Lautrbach
2021-01-14 14:30 ` William Roberts
0 siblings, 1 reply; 6+ messages in thread
From: Petr Lautrbach @ 2021-01-14 13:39 UTC (permalink / raw)
To: selinux; +Cc: Petr Lautrbach
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] libselinux: Always close status page fd
2021-01-14 13:39 [PATCH] libselinux: Always close status page fd Petr Lautrbach
@ 2021-01-14 14:30 ` William Roberts
2021-01-14 14:33 ` William Roberts
2021-01-14 14:54 ` Petr Lautrbach
0 siblings, 2 replies; 6+ messages in thread
From: William Roberts @ 2021-01-14 14:30 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: SElinux list
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libselinux: Always close status page fd
2021-01-14 14:30 ` William Roberts
@ 2021-01-14 14:33 ` William Roberts
2021-01-14 14:46 ` William Roberts
2021-01-14 14:54 ` Petr Lautrbach
1 sibling, 1 reply; 6+ messages in thread
From: William Roberts @ 2021-01-14 14:33 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: SElinux list
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libselinux: Always close status page fd
2021-01-14 14:33 ` William Roberts
@ 2021-01-14 14:46 ` William Roberts
0 siblings, 0 replies; 6+ messages in thread
From: William Roberts @ 2021-01-14 14:46 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: SElinux list
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libselinux: Always close status page fd
2021-01-14 14:30 ` William Roberts
2021-01-14 14:33 ` William Roberts
@ 2021-01-14 14:54 ` Petr Lautrbach
2021-01-14 15:11 ` William Roberts
1 sibling, 1 reply; 6+ messages in thread
From: Petr Lautrbach @ 2021-01-14 14:54 UTC (permalink / raw)
To: SElinux list; +Cc: William Roberts
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libselinux: Always close status page fd
2021-01-14 14:54 ` Petr Lautrbach
@ 2021-01-14 15:11 ` William Roberts
0 siblings, 0 replies; 6+ messages in thread
From: William Roberts @ 2021-01-14 15:11 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: SElinux list
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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-14 15:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-14 13:39 [PATCH] libselinux: Always close status page fd Petr Lautrbach
2021-01-14 14:30 ` William Roberts
2021-01-14 14:33 ` William Roberts
2021-01-14 14:46 ` William Roberts
2021-01-14 14:54 ` Petr Lautrbach
2021-01-14 15:11 ` William Roberts
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).