[PATCH rtems-libbsd 1/2] racoon/session: Honor file descriptor maximum

[Chris Johns]

On 1/3/21 7:24 pm, Christian MAUDERER wrote:
> Hello Chris,
> 
> thanks for the review.
> 
> Am 26.02.21 um 19:04 schrieb Chris Johns:
>> On 26/2/21 2:01 am, Christian Mauderer wrote:
>>> Dynamically allocate a big enough file descriptor set for select(). A
>>> better solution would be to use kqueue() instead of select().
>>> ---
>>>   .../racoon/rtems-bsd-racoon-session-data.h    |  6 +--
>>>   ipsec-tools/src/racoon/session.c              | 40 +++++++++++++++++++
>>>   2 files changed, 43 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/ipsec-tools/src/racoon/rtems-bsd-racoon-session-data.h
>>> b/ipsec-tools/src/racoon/rtems-bsd-racoon-session-data.h
>>> index b869a1518..196107a35 100644
>>> --- a/ipsec-tools/src/racoon/rtems-bsd-racoon-session-data.h
>>> +++ b/ipsec-tools/src/racoon/rtems-bsd-racoon-session-data.h
>>> @@ -2,11 +2,11 @@
>>>   #include <rtems/linkersets.h>
>>>   #include "rtems-bsd-racoon-data.h"
>>>   /* session.c */
>>> -RTEMS_LINKER_RWSET_CONTENT(bsd_prog_racoon, static fd_set active_mask);
>>> -RTEMS_LINKER_RWSET_CONTENT(bsd_prog_racoon, static fd_set preset_mask);
>>> +RTEMS_LINKER_RWSET_CONTENT(bsd_prog_racoon, static _types_fd_set
>>> *allocated_active_mask);
>>> +RTEMS_LINKER_RWSET_CONTENT(bsd_prog_racoon, static _types_fd_set
>>> *allocated_preset_mask);
>>>   RTEMS_LINKER_RWSET_CONTENT(bsd_prog_racoon, static int nfds);
>>>   RTEMS_LINKER_RWSET_CONTENT(bsd_prog_racoon, static int signals[]);
>>>   RTEMS_LINKER_RWSET_CONTENT(bsd_prog_racoon, static sig_atomic_t volatile
>>> volatile sigreq[]);
>>> -RTEMS_LINKER_RWSET_CONTENT(bsd_prog_racoon, static struct fd_monitor
>>> fd_monitors[]);
>>> +RTEMS_LINKER_RWSET_CONTENT(bsd_prog_racoon, static struct fd_monitor
>>> *allocated_fd_monitors);
>>>   RTEMS_LINKER_RWSET_CONTENT(bsd_prog_racoon, static struct fd_monitor_list
>>> fd_monitor_tree[]);
>>>   RTEMS_LINKER_RWSET_CONTENT(bsd_prog_racoon, static struct sched scflushsa);
>>> diff --git a/ipsec-tools/src/racoon/session.c b/ipsec-tools/src/racoon/session.c
>>> index 65124c15e..90120c761 100644
>>> --- a/ipsec-tools/src/racoon/session.c
>>> +++ b/ipsec-tools/src/racoon/session.c
>>> @@ -65,6 +65,10 @@
>>>   #include <sys/stat.h>
>>>   #include <paths.h>
>>>   #include <err.h>
>>> +#ifdef __rtems__
>>> +#include <sys/param.h>
>>> +#include <rtems/libio_.h>
>>> +#endif /* __rtems__ */
>>>     #include <netinet/in.h>
>>>   #include <resolv.h>
>>> @@ -123,8 +127,16 @@ static void check_sigreq __P((void));
>>>   static void check_flushsa __P((void));
>>>   static int close_sockets __P((void));
>>>   +#ifndef __rtems__
>>>   static fd_set preset_mask, active_mask;
>>>   static struct fd_monitor fd_monitors[FD_SETSIZE];
>>> +#else /* __rtems__ */
>>> +static fd_set *allocated_preset_mask, *allocated_active_mask;
>>> +static struct fd_monitor *allocated_fd_monitors;
>>> +#define preset_mask (*allocated_preset_mask)
>>> +#define active_mask (*allocated_active_mask)
>>> +#define fd_monitors (allocated_fd_monitors)
>>> +#endif /* __rtems__ */
>>>   static TAILQ_HEAD(fd_monitor_list, fd_monitor)
>>> fd_monitor_tree[NUM_PRIORITIES];
>>>   static int nfds = 0;
>>>   @@ -134,7 +146,11 @@ static struct sched scflushsa = SCHED_INITIALIZER();
>>>   void
>>>   monitor_fd(int fd, int (*callback)(void *, int), void *ctx, int priority)
>>>   {
>>> +#ifndef __rtems__
>>>       if (fd < 0 || fd >= FD_SETSIZE) {
>>> +#else /* __rtems__ */
>>> +    if (fd < 0 || fd >= rtems_libio_number_iops) {
>>> +#endif /* __rtems__ */
>>>           plog(LLV_ERROR, LOCATION, NULL, "fd_set overrun");
>>>           exit(1);
>>>       }
>>> @@ -158,7 +174,11 @@ monitor_fd(int fd, int (*callback)(void *, int), void
>>> *ctx, int priority)
>>>   void
>>>   unmonitor_fd(int fd)
>>>   {
>>> +#ifndef __rtems__
>>>       if (fd < 0 || fd >= FD_SETSIZE) {
>>> +#else /* __rtems__ */
>>> +    if (fd < 0 || fd >= rtems_libio_number_iops) {
>>> +#endif /* __rtems__ */
>>>           plog(LLV_ERROR, LOCATION, NULL, "fd_set overrun");
>>>           exit(1);
>>>       }
>>> @@ -186,7 +206,22 @@ session(void)
>>>       struct fd_monitor *fdm;
>>>         nfds = 0;
>>> +#ifndef __rtems__
>>>       FD_ZERO(&preset_mask);
>>> +#else /* __rtems__ */
>>> +    allocated_preset_mask = calloc(sizeof(fd_set),
>>> +        howmany(rtems_libio_number_iops, sizeof(fd_set) * 8));
>>
>> Does `maxfiles` work here?
>>
> 
> To be honest: I'm not sure.
> 
> According to the comment in file.h:
> 
> extern int maxfiles; /* kernel limit on number of open files */
> 

Yes.

> Sounds like it _can_ be the same as the maximum file number but doesn't have to.

I think we need to have them be the same value.

> I didn't find where we implement it. It's declared as an extern int maxfiles but
> I didn't find any definition. I found it only in libbsd in
> freebsd/sys/kern/uipc_socket.c where it is defined like follows:
> 
> #define maxfiles rtems_libio_number_iops

Ah OK. I knew it had been assigned somewhere and yes it looks like it is local
to that file.

> 
> So question is: Where and how is maxfiles defined?
>

I have provided a value in the rtemsbsd init file as part of the set og globals
we need to maintained.

>>> +    if (allocated_preset_mask == NULL)
>>> +        errx(1, "failed to allocate preset_mask");
>>> +    allocated_active_mask = calloc(sizeof(fd_set),
>>> +        howmany(rtems_libio_number_iops, sizeof(fd_set) * 8));
>>> +    if (allocated_active_mask == NULL)
>>> +        errx(1, "failed to allocate active_mask");
>>> +    allocated_fd_monitors = calloc(
>>> +        rtems_libio_number_iops, sizeof(struct fd_monitor));
>>> +    if (allocated_fd_monitors == NULL)
>>> +        errx(1, "failed to allocate fd_monitors");
>>
>> At the core of this issue is the rotating fd allocation that we have in libio. A
>> report from a FreeBSD machine I have is:
>>
>> $ sysctl -a | grep maxfiles
>> kern.maxfiles: 1037243
>> kern.maxfilesperproc: 933516
>>
>> I doubt a select process in FreeBSD needs a select array with that many bits. I
>> have added similar code else where but I am wondering if this is really what we
>> want to do. A side effect for us is the stack usage is not bounded and that is a
>> problem. I think our newlib based max setting is too small.
> 
> I think we have to distinguish between FreeBSD kernel space and user space. If I
> have seen it correctly, FreeBSD uses kqueues or maybe sometimes poll instead of
> select in kernel space most of the time. That avoids the problem with big file
> numbers.

Yes kqueue is nice but we need to support existing code and this is a primary
role libbsd needs to perform. The lack is signals in libbsd is another source of
issue.

> Default for FreeBSD seems to be a FD_SETSIZE of 1024. See the Notes section in
> the select man page:
> 
> https://www.freebsd.org/cgi/man.cgi?query=select&sektion=2&manpath=freebsd-release-ports#end
> 

Yes.

> I think select is used in user space applications most of the time. I would
> guess that FreeBSD has some file number mapping between kernel and user space
> that would allow every application to open 1024 files.

Yes. There are other checks and arrays in the kernel's select that handle the
size of 1024 or smaller. We should align ourselves with FreeBSD. A select call
uses a lot of stack.

>> FYI I have a major set of changes to libbsd that enables FreeBSD descriptor
>> support and moves the libio support to specific interfaces.
> 
> OK. When do you plan to add that and to which branches?

I am looking 6-freebsd-12 and master. Thses changes are a major rework and not
suitable for 5.

> I would like to add this
> patch set to 5 sooner or later because it fixes a potential bug in racoon for
> that branch too.

Sorry I missed this for 5. I will need to think about this on 5. I do not think
it is a problem but I am not sure about it on 6 and beyond. I would prefer we
consider a better fix for select that the limited 64 descriptors we currently have.

>> The changes I have
>> made are in tarballs in nfsv4 in my ftp area. The support includes the mapping
>> of libio descriptors to FB ones. The benefit of these changes is VFS and FreeBSD
>> file system support.
> 
> FreeBSD file system support sounds like a great feature. It would allow access
> to a whole bunch of stable and well tested file system implementations.

It does. I have NFSv4 up and running.

Chris