Re: Use SMR instead of SRP list in rtsock.c

[Vitaliy Makkoveev]

On Thu, Jun 30, 2022 at 11:56:55AM +0200, Claudio Jeker wrote:
> On Thu, Jun 30, 2022 at 12:34:33PM +0300, Vitaliy Makkoveev wrote:
> > On Thu, Jun 30, 2022 at 11:08:48AM +0200, Claudio Jeker wrote:
> > > This diff converts the SRP list to a SMR list in rtsock.c
> > > SRP is a bit strange with how it works and the SMR code is a bit easier to
> > > understand. Since we can sleep in the SMR_TAILQ_FOREACH() we need to grab
> > > a refcount on the route pcb so that we can leave the SMR critical section
> > > and then enter the SMR critical section at the end of the loop before
> > > dropping the refcount again.
> > > 
> > > The diff does not immeditaly explode but I doubt we can exploit
> > > parallelism in route_input() so this may fail at some later stage if it is
> > > wrong.
> > > 
> > > Comments from the lock critics welcome
> > 
> > We use `so_lock' rwlock(9) to protect route domain sockets. We can't
> > convert this SRP list to SMR list because we call solock() within
> > foreach loop.
> 
> because of the so_lock the code uses a refcnt on the route pcb to make
> sure that the object is not freed while we sleep. So that is handled by
> this diff.
>  
> > We can easily crash kernel by running in parallel some "route monitor"
> > commands and "while true; ifconfig vether0 create ; ifconfig vether0
> > destroy; done".
> 
> That does not cause problem on my system.
>  

Sorry, I missed you leave SMR section before solock() call:

> > > @@ -519,7 +498,8 @@ route_input(struct mbuf *m0, struct sock
> > >               rop->rop_proto != sa_family)
> > >                   continue;
> > >  
> > > -
> > > +         refcnt_take(&rop->rop_refcnt);
> > > +         smr_read_leave();
> > >           so = rop->rop_socket;
> > >           solock(so);
> > >  

My system is stable with the second version of this diff and the
following test.

#include <sys/ioctl.h>
#include <sys/select.h>
#include <sys/socket.h>
#include <net/if.h>
#include <stdio.h>
#include <stdlib.h>
#include <err.h>
#include <errno.h>
#include <pthread.h>
#include <string.h>
#include <unistd.h>

static struct ifreq ifr;

static void *clone(void *arg)
{
        int s;

        if ((s = socket(AF_INET, SOCK_DGRAM, 0)) < 0)
                err(1, "socket");
        while (1) {
                if (ioctl(s, SIOCIFCREATE, &ifr) < 0)
                        if (errno == EINVAL)
                                err(1, "ioctl");
                if(ioctl(s, SIOCIFDESTROY, &ifr)<0)
                        if(errno == EINVAL)
                                err(1, "ioctl");
        }

        return NULL;
}

static void *rtsock(void *arg)
{
        int s;

        while (1){
                if ((s = socket(AF_ROUTE, SOCK_RAW, 0)) < 0)
                        err(1, "socket");
                close(s);
        }

        return NULL;
}

int main(int argc, char *argv[])
{
        pthread_t thr;
        int i;

        if( argc != 2) {
                fprintf(stderr, "usage: %s ifname\n", getprogname());
                return 1;
        }

        if (getuid() != 0) {
                fprintf(stderr, "should be root\n");
                return 1;
        }

        memset(&ifr, 0, sizeof(ifr));
        strlcpy(ifr.ifr_name, argv[1], sizeof(ifr.ifr_name));

        for(i = 0; i < 8 * 4; ++i) {
                if (pthread_create(&thr, NULL, clone, NULL)!=0)
                        errx(1, "pthread_create");
        }

        for (i = 0; i < 8 * 4; ++i) {
                if (pthread_create(&thr, NULL, rtsock, NULL) != 0)
                        errx(1, "pthread_create");
        }

        select(0, NULL, NULL, NULL, NULL);

        return 0;
}