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?
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.
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?
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 3 May 2023 21:55:21 -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() */
> @@ -96,6 +95,8 @@ pf_osfp_fingerprint(struct pf_pdesc *pd)
> struct ip6_hdr *ip6 = NULL;
> char hdr[60];
>
> + PF_ASSERT_LOCKED();
> +
> if (pd->proto != IPPROTO_TCP)
> return (NULL);
>
> @@ -312,7 +313,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 +323,6 @@ pf_osfp_flush(void)
> pool_put(&pf_osfp_pl, fp);
> }
> PF_UNLOCK();
> - NET_UNLOCK();
> }
>
>
> @@ -379,14 +378,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))) {
> 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);
> @@ -416,7 +413,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()))
> @@ -553,7 +549,6 @@ pf_osfp_get(struct pf_osfp_ioctl *fpioc)
>
>
> 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 +563,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);
> }
> @@ -585,6 +578,8 @@ struct pf_os_fingerprint *
> 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));
