On Wed, 2023-04-26 at 13:47 +0200, Alexandr Nedvedicky wrote: > Hello, > > </snip> > On Wed, Apr 26, 2023 at 11:37:58AM +1000, David Gwynne wrote: > > > fail: > > > - if (flags & FWRITE) > > > - rw_exit_write(&pfioctl_rw); > > > - else > > > - rw_exit_read(&pfioctl_rw); > > > + rw_exit_write(&pfioctl_rw); > > > > i dont think having the open mode flags affect whether you take a shared > > or exclusive lock makes sense anyway, so im happy with these bits. > > while updating my diff I've noticed pfclose() needs > same change: dropping 'flags & FWRITE' test. This is > fixed i new diff below. > > </snip> > > > > int unit = minor(dev); > > > > if (unit & ((1 << CLONE_SHIFT) - 1)) > > return (ENXIO); > > > > this has some ties into the second issue, which is that we shouldn't > > use the pid as an identifier for state across syscalls like this > > in the kernel. the reason is that userland could be weird or buggy > > (or hostile) and fork in between creating a transaction and ending > > a transaction, but it will still have the fd it from from open(/dev/pf). > > instead, we should be using the whole dev_t or the minor number, > > as long as it includes the bits that the cloning infrastructure > > uses, as the identifier. > > in new diff I'm using a minor(dev) to identify transaction owner. > > > > i would also check that the process^Wminor number that created a > > transaction is the same when looking up a transaction in pf_find_trans. > > the pf_find_trans() is a dead code in diff below as it is > not being used there. Just to make sure I got things right > so I can update '3/3' diff in stack accordingly. Let's assume > snippet below comes from pfioctl() function > > int > > > > pfioctl(dev_t dev, u_long cmd, caddr_t addr, int flags, struct proc *p) { > ... > t = pf_find_trans(ticket); > if (t == NULL) > return (ENXIO); > > KASSERT(t->pft_unit == minor(dev)); > > is that kind of check in KASSET() something you have on your mind? > perhaps I can trade KASSERT() to regular code: > > if (t->pft_unit != minor(dev)) > return (EPERM); > > > thank you for pointers to bpf.c updated diff is below. > > regards > sashan > > --------8<---------------8<---------------8<------------------8<-------- > diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c > index 1141069dcf6..b9904c545c5 100644 > --- a/sys/net/pf_ioctl.c > +++ b/sys/net/pf_ioctl.c > @@ -117,6 +117,11 @@ void pf_qid_unref(u_int16_t); > int pf_states_clr(struct pfioc_state_kill *); > int pf_states_get(struct pfioc_states *); > > +struct pf_trans *pf_open_trans(uint32_t); > +struct pf_trans *pf_find_trans(uint64_t); > +void pf_free_trans(struct pf_trans *); > +void pf_rollback_trans(struct pf_trans *); > + > struct pf_rule pf_default_rule, pf_default_rule_new; > > struct { > @@ -168,6 +173,8 @@ int pf_rtlabel_add(struct pf_addr_wrap > *); > void pf_rtlabel_remove(struct pf_addr_wrap *); > void pf_rtlabel_copyout(struct pf_addr_wrap *); > > +uint64_t trans_ticket = 1; > +LIST_HEAD(, pf_trans) pf_ioctl_trans = LIST_HEAD_INITIALIZER(pf_trans); > > void > pfattach(int num) > @@ -293,6 +300,28 @@ pfopen(dev_t dev, int flags, int fmt, struct proc *p) > int > pfclose(dev_t dev, int flags, int fmt, struct proc *p) > { > + struct pf_trans *w, *s; > + LIST_HEAD(, pf_trans) tmp_list; > + uint32_t unit = minor(dev); > + > + if (minor(dev) >= 1) > + return (ENXIO);
If you want to use the minor dev-id to release the transaction, the above two lines should go away. > + > + LIST_INIT(&tmp_list); > + rw_enter_write(&pfioctl_rw); > + LIST_FOREACH_SAFE(w, &pf_ioctl_trans, pft_entry, s) { > + if (w->pft_unit == unit) { > + LIST_REMOVE(w, pft_entry); > + LIST_INSERT_HEAD(&tmp_list, w, pft_entry); > + } > + } > + rw_exit_write(&pfioctl_rw); > + > + while ((w = LIST_FIRST(&tmp_list)) != NULL) { > + LIST_REMOVE(w, pft_entry); > + pf_free_trans(w); > + } > + > return (0); > } > > @@ -1142,10 +1171,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int > flags, struct proc *p) > return (EACCES); > } > > - if (flags & FWRITE) > - rw_enter_write(&pfioctl_rw); > - else > - rw_enter_read(&pfioctl_rw); > + rw_enter_write(&pfioctl_rw); > > switch (cmd) { > > @@ -3022,10 +3048,7 @@ pfioctl(dev_t dev, u_long cmd, caddr_t addr, int > flags, struct proc *p) > break; > } > fail: > - if (flags & FWRITE) > - rw_exit_write(&pfioctl_rw); > - else > - rw_exit_read(&pfioctl_rw); > + rw_exit_write(&pfioctl_rw); > > return (error); > } > @@ -3244,3 +3267,52 @@ pf_sysctl(void *oldp, size_t *oldlenp, void *newp, > size_t newlen) > > return sysctl_rdstruct(oldp, oldlenp, newp, &pfs, sizeof(pfs)); > } > + > +struct pf_trans * > +pf_open_trans(uint32_t unit) > +{ > + static uint64_t ticket = 1; > + struct pf_trans *t; > + > + rw_assert_wrlock(&pfioctl_rw); > + > + t = malloc(sizeof(*t), M_TEMP, M_WAITOK); > + memset(t, 0, sizeof(struct pf_trans)); > + t->pft_unit = unit; > + t->pft_ticket = ticket++; > + > + LIST_INSERT_HEAD(&pf_ioctl_trans, t, pft_entry); > + > + return (t); > +} > + > +struct pf_trans * > +pf_find_trans(uint64_t ticket) > +{ > + struct pf_trans *t; > + > + rw_assert_anylock(&pfioctl_rw); > + > + LIST_FOREACH(t, &pf_ioctl_trans, pft_entry) { > + if (t->pft_ticket == ticket) > + break; > + } > + > + return (t); > +} > + > +void > +pf_free_trans(struct pf_trans *t) > +{ > + free(t, M_TEMP, sizeof(*t)); > +} > + > +void > +pf_rollback_trans(struct pf_trans *t) > +{ > + if (t != NULL) { > + rw_assert_wrlock(&pfioctl_rw); > + LIST_REMOVE(t, pft_entry); > + pf_free_trans(t); > + } > +} > diff --git a/sys/net/pfvar_priv.h b/sys/net/pfvar_priv.h > index 38fff6473aa..5af2027733a 100644 > --- a/sys/net/pfvar_priv.h > +++ b/sys/net/pfvar_priv.h > @@ -322,6 +322,17 @@ enum { > > extern struct cpumem *pf_anchor_stack; > > +enum pf_trans_type { > + PF_TRANS_NONE, > + PF_TRANS_MAX > +}; > + > +struct pf_trans { > + LIST_ENTRY(pf_trans) pft_entry; > + uint32_t pft_unit; /* process id */ > + uint64_t pft_ticket; > + enum pf_trans_type pft_type; > +}; > extern struct task pf_purge_task; > extern struct timeout pf_purge_to; > >
smime.p7s
Description: S/MIME cryptographic signature