On Wed, Apr 26, 2023 at 09:49:18AM +0000, Gerhard Roth wrote:
> On Wed, 2023-04-26 at 19:42 +1000, David Gwynne wrote:
> > 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:
>
> Sorry for the noise. I missed this. Thanks for helping me!
all good. it was pretty painful when i discovered the multiple opens to
one close thing so i understand wanting to warn people.
>
>
> >
> > > 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.
>
