On Wed, Apr 26, 2023 at 07:48:18AM +0000, Gerhard Roth wrote:
> On Wed, 2023-04-26 at 00:39 +0200, Alexandr Nedvedicky wrote:
> > Hello,
> >
> > This is the second diff. It introduces a transaction (pf_trans).
> > It's more or less diff with dead code.
> >
> > It's still worth to note those two chunks in this diff:
> >
> > @@ -1142,10 +1172,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 +3049,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);
> > ??
> > `pfioctl_rw` serializes processes which perform ioctl(2) to pf(4).
> > I'd like to also this lock to serialize access to table of transactions.
> > To keep things simple for now I'd like to make every process to perform
> > ioctl(2) on pf(4) exclusively. I plan to revisit this later when I'll
> > be pushing operation on pfioctl_rw further down to individual ioctl
> > operations.
> >
> > the operations pf_open_trans(), pf_find_trans(), pf_rollback_trans()
> > introduced in this diff are unused. They will be brought alive in
> > the 3rd diff.
> >
> > thanks and
> > regards
> > sashan
> >
> > --------8<---------------8<---------------8<------------------8<--------
> > diff --git a/sys/net/pf_ioctl.c b/sys/net/pf_ioctl.c
> > index 7ea22050506..7e4c7d2a2ab 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(pid_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,29 @@ 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;
> > +
> > +??????????????if (minor(dev) >= 1)
> > +??????????????????????????????return (ENXIO);
> > +
> > +??????????????if (flags & FWRITE) {
> > +??????????????????????????????LIST_INIT(&tmp_list);
> > +??????????????????????????????rw_enter_write(&pfioctl_rw);
> > +??????????????????????????????LIST_FOREACH_SAFE(w, &pf_ioctl_trans,
> > pft_entry, s) {
> > +??????????????????????????????????????????????if (w->pft_pid ==
> > p->p_p->ps_pid) {
> > +??????????????????????????????????????????????????????????????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);
> > ??}
>
> Kernel close() routines don't work the same way as user-land close() ones do.
> pfclose() is called only once when the last reference to /dev/pf goes away.
> There is no way you can keep track of your transactions like that.
we made /dev/pf clonable in src/sys/sys/conf.h r1.159 so we could do
this. the commit message was:
> make /dev/pf a clonable device.
>
> this provides a 1:1 relationship of pfopen() calls to pfclose()
> calls. in turn, this makes it a lot easier to track stuff allocated
> by a process and then clean it up if that process goes away
> unexpectedly. the unique dev_t provided by the cloning machinery
> gives us a good identifier to track this state with too.
