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.
>
> @@ -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);
>
> return (error);
> }
> @@ -3244,3 +3268,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(pid_t pid)
> +{
> + 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_pid = pid;
> + 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..c180b22df5c 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;
> + pid_t pft_pid; /* 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
