On Sat, May 06, 2023 at 08:56:06PM +0000, Klemens Nanni wrote:
> On Sat, May 06, 2023 at 09:33:05PM +0200, Alexander Bluhm wrote:
> > On Sat, May 06, 2023 at 11:11:25AM +0000, Klemens Nanni wrote:
> > > pf_osfp.c contains all the locking for these three ioctls, this removes
> > > the net lock from it.
> > >
> > > All data is protected by the pf lock, new asserts verify that.
> > >
> > > Beside the pf ioctl handler, pf_match_rule()'s call to pf_osfp_match()
> > > is the only hook into it.
> > >
> > > tcpbump still compiles pf_osfp.c without object change.
> > >
> > > Feedback? OK?
> >
> > Could you document that pf_osfp_list and fp_next is protected by
> > pf lock?
>
> Picked [p] for pf_lock so as to not collide with pfvar_priv.h's comment
> about "Protection/owernership of pf_state members".
>
> > There is nothing in pf_osfp_fingerprint() that needs pf lock directly.
> > The critical access is the SLIST_FOREACH in pf_osfp_find(). Place
> > the PF_ASSERT_LOCKED() there.
>
> Misplaced, thanks.
>
> > Why is pf_osfp_list passed to pf_osfp_find() and pf_osfp_find_exact()?
> > Can we remove the parameter and just use pf_osfp_list?
>
> Wanted to clean this up in a separate diff, but we might as well do
> everything in one go.
OK bluhm@
> Index: pf_osfp.c
> ===================================================================
> RCS file: /cvs/src/sys/net/pf_osfp.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 pf_osfp.c
> --- pf_osfp.c 15 Dec 2020 15:23:48 -0000 1.45
> +++ pf_osfp.c 6 May 2023 20:52:07 -0000
> @@ -60,10 +60,9 @@ typedef struct pool pool_t;
> #define pool_put(pool, item) free(item)
> #define pool_init(pool, size, a, ao, f, m, p) (*(pool)) = (size)
>
> -#define NET_LOCK()
> -#define NET_UNLOCK()
> #define PF_LOCK()
> #define PF_UNLOCK()
> +#define PF_ASSERT_LOCKED()
>
> #ifdef PFDEBUG
> #include <sys/stdarg.h> /* for DPFPRINTF() */
> @@ -71,16 +70,21 @@ typedef struct pool pool_t;
>
> #endif /* _KERNEL */
>
> -SLIST_HEAD(pf_osfp_list, pf_os_fingerprint) pf_osfp_list;
> -pool_t pf_osfp_entry_pl;
> -pool_t pf_osfp_pl;
> -
> -struct pf_os_fingerprint *pf_osfp_find(struct pf_osfp_list *,
> - struct pf_os_fingerprint *, u_int8_t);
> -struct pf_os_fingerprint *pf_osfp_find_exact(struct pf_osfp_list *,
> - struct pf_os_fingerprint *);
> -void pf_osfp_insert(struct pf_osfp_list *,
> - struct pf_os_fingerprint *);
> +/*
> + * Protection/ownership:
> + * I immutable after pf_osfp_initialize()
> + * p pf_lock
> + */
> +
> +SLIST_HEAD(pf_osfp_list, pf_os_fingerprint) pf_osfp_list =
> + SLIST_HEAD_INITIALIZER(pf_osfp_list); /* [p] */
> +pool_t pf_osfp_entry_pl; /* [I] */
> +pool_t pf_osfp_pl; /* [I] */
> +
> +struct pf_os_fingerprint *pf_osfp_find(struct pf_os_fingerprint *,
> + u_int8_t);
> +struct pf_os_fingerprint *pf_osfp_find_exact(struct pf_os_fingerprint *);
> +void pf_osfp_insert(struct pf_os_fingerprint *);
>
>
> #ifdef _KERNEL
> @@ -257,8 +261,7 @@ pf_osfp_fingerprint_hdr(const struct ip
> (fp.fp_flags & PF_OSFP_WSCALE_DC) ? "*" : "",
> fp.fp_wscale);
>
> - if ((fpresult = pf_osfp_find(&pf_osfp_list, &fp,
> - PF_OSFP_MAXTTL_OFFSET)))
> + if ((fpresult = pf_osfp_find(&fp, PF_OSFP_MAXTTL_OFFSET)))
> return (&fpresult->fp_oses);
> return (NULL);
> }
> @@ -302,7 +305,6 @@ pf_osfp_initialize(void)
> IPL_NONE, PR_WAITOK, "pfosfpen", NULL);
> pool_init(&pf_osfp_pl, sizeof(struct pf_os_fingerprint), 0,
> IPL_NONE, PR_WAITOK, "pfosfp", NULL);
> - SLIST_INIT(&pf_osfp_list);
> }
>
> /* Flush the fingerprint list */
> @@ -312,7 +314,6 @@ pf_osfp_flush(void)
> struct pf_os_fingerprint *fp;
> struct pf_osfp_entry *entry;
>
> - NET_LOCK();
> PF_LOCK();
> while ((fp = SLIST_FIRST(&pf_osfp_list))) {
> SLIST_REMOVE_HEAD(&pf_osfp_list, fp_next);
> @@ -323,7 +324,6 @@ pf_osfp_flush(void)
> pool_put(&pf_osfp_pl, fp);
> }
> PF_UNLOCK();
> - NET_UNLOCK();
> }
>
>
> @@ -379,14 +379,12 @@ pf_osfp_add(struct pf_osfp_ioctl *fpioc)
> return (ENOMEM);
> }
>
> - NET_LOCK();
> PF_LOCK();
> - if ((fp = pf_osfp_find_exact(&pf_osfp_list, &fpadd))) {
> + if ((fp = pf_osfp_find_exact(&fpadd))) {
> struct pf_osfp_entry *tentry;
>
> SLIST_FOREACH(tentry, &fp->fp_oses, fp_entry) {
> if (PF_OSFP_ENTRY_EQ(tentry, &fpioc->fp_os)) {
> - NET_UNLOCK();
> PF_UNLOCK();
> pool_put(&pf_osfp_entry_pl, entry);
> pool_put(&pf_osfp_pl, fp_prealloc);
> @@ -405,7 +403,7 @@ pf_osfp_add(struct pf_osfp_ioctl *fpioc)
> fp->fp_wscale = fpioc->fp_wscale;
> fp->fp_ttl = fpioc->fp_ttl;
> SLIST_INIT(&fp->fp_oses);
> - pf_osfp_insert(&pf_osfp_list, fp);
> + pf_osfp_insert(fp);
> }
> memcpy(entry, &fpioc->fp_os, sizeof(*entry));
>
> @@ -416,7 +414,6 @@ pf_osfp_add(struct pf_osfp_ioctl *fpioc)
>
> SLIST_INSERT_HEAD(&fp->fp_oses, entry, fp_entry);
> PF_UNLOCK();
> - NET_UNLOCK();
>
> #ifdef PFDEBUG
> if ((fp = pf_osfp_validate()))
> @@ -433,11 +430,12 @@ pf_osfp_add(struct pf_osfp_ioctl *fpioc)
>
> /* Find a fingerprint in the list */
> struct pf_os_fingerprint *
> -pf_osfp_find(struct pf_osfp_list *list, struct pf_os_fingerprint *find,
> - u_int8_t ttldiff)
> +pf_osfp_find(struct pf_os_fingerprint *find, u_int8_t ttldiff)
> {
> struct pf_os_fingerprint *f;
>
> + PF_ASSERT_LOCKED();
> +
> #define MATCH_INT(_MOD, _DC, _field) \
> if ((f->fp_flags & _DC) == 0) { \
> if ((f->fp_flags & _MOD) == 0) { \
> @@ -449,7 +447,7 @@ pf_osfp_find(struct pf_osfp_list *list,
> } \
> }
>
> - SLIST_FOREACH(f, list, fp_next) {
> + SLIST_FOREACH(f, &pf_osfp_list, fp_next) {
> if (f->fp_tcpopts != find->fp_tcpopts ||
> f->fp_optcnt != find->fp_optcnt ||
> f->fp_ttl < find->fp_ttl ||
> @@ -507,11 +505,13 @@ pf_osfp_find(struct pf_osfp_list *list,
>
> /* Find an exact fingerprint in the list */
> struct pf_os_fingerprint *
> -pf_osfp_find_exact(struct pf_osfp_list *list, struct pf_os_fingerprint *find)
> +pf_osfp_find_exact(struct pf_os_fingerprint *find)
> {
> struct pf_os_fingerprint *f;
>
> - SLIST_FOREACH(f, list, fp_next) {
> + PF_ASSERT_LOCKED();
> +
> + SLIST_FOREACH(f, &pf_osfp_list, fp_next) {
> if (f->fp_tcpopts == find->fp_tcpopts &&
> f->fp_wsize == find->fp_wsize &&
> f->fp_psize == find->fp_psize &&
> @@ -528,18 +528,20 @@ pf_osfp_find_exact(struct pf_osfp_list *
>
> /* Insert a fingerprint into the list */
> void
> -pf_osfp_insert(struct pf_osfp_list *list, struct pf_os_fingerprint *ins)
> +pf_osfp_insert(struct pf_os_fingerprint *ins)
> {
> struct pf_os_fingerprint *f, *prev = NULL;
>
> + PF_ASSERT_LOCKED();
> +
> /* XXX need to go semi tree based. can key on tcp options */
>
> - SLIST_FOREACH(f, list, fp_next)
> + SLIST_FOREACH(f, &pf_osfp_list, fp_next)
> prev = f;
> if (prev)
> SLIST_INSERT_AFTER(prev, ins, fp_next);
> else
> - SLIST_INSERT_HEAD(list, ins, fp_next);
> + SLIST_INSERT_HEAD(&pf_osfp_list, ins, fp_next);
> }
>
> /* Fill a fingerprint by its number (from an ioctl) */
> @@ -551,9 +553,7 @@ pf_osfp_get(struct pf_osfp_ioctl *fpioc)
> int num = fpioc->fp_getnum;
> int i = 0;
>
> -
> memset(fpioc, 0, sizeof(*fpioc));
> - NET_LOCK();
> PF_LOCK();
> SLIST_FOREACH(fp, &pf_osfp_list, fp_next) {
> SLIST_FOREACH(entry, &fp->fp_oses, fp_entry) {
> @@ -568,13 +568,11 @@ pf_osfp_get(struct pf_osfp_ioctl *fpioc)
> memcpy(&fpioc->fp_os, entry,
> sizeof(fpioc->fp_os));
> PF_UNLOCK();
> - NET_UNLOCK();
> return (0);
> }
> }
> }
> PF_UNLOCK();
> - NET_UNLOCK();
>
> return (EBUSY);
> }
> @@ -586,6 +584,8 @@ pf_osfp_validate(void)
> {
> struct pf_os_fingerprint *f, *f2, find;
>
> + PF_ASSERT_LOCKED();
> +
> SLIST_FOREACH(f, &pf_osfp_list, fp_next) {
> memcpy(&find, f, sizeof(find));
>
> @@ -598,7 +598,7 @@ pf_osfp_validate(void)
> find.fp_wsize *= (find.fp_mss + 40);
> else if (f->fp_flags & PF_OSFP_WSIZE_MOD)
> find.fp_wsize *= 2;
> - if (f != (f2 = pf_osfp_find(&pf_osfp_list, &find, 0))) {
> + if (f != (f2 = pf_osfp_find(&find, 0))) {
> if (f2)
> DPFPRINTF(LOG_NOTICE,
> "Found \"%s %s %s\" instead of "
