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! > > > 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.
smime.p7s
Description: S/MIME cryptographic signature
