On Sun, Nov 20, 2022 at 02:15:24AM +0100, Alexandr Nedvedicky wrote:
> Hello Olivier,
>
> thank your for reporting a bug. Patch is always welcomed,
> though I think there is a better way to fix it.
>
> I was able to reproduce the bug. After adding a 64 groups to
> interface vio0 I was getting 'Bad Address' too.
>
> On Fri, Nov 18, 2022 at 06:09:38PM +0100, Olivier Croquin wrote:
> </snip>
> >
> > In the fix proposed below, I choose arbitrarily to set the
> > pfrb_size to two times the number of interfaces found whith getifaddrs.
> > Most of the times, it will be too large, but, with this value, we
> > are sure to handle all the interfaces and interfaces groups.
> >
> > An other option. The DIOCIGETIFACES ioctl command could behave as
> > DIOCRGETTABLES when the buffer is too small (cf. man pf) :
> > "f the buffer is too small, the kernel does not store anything but
> > just returns the required buffer size, without error".
> >
>
> the interesting thing is that 'other option' is almost implemented in
> pf(4) already. Unfortunately there is kind of off-by-one bug. Diff below
> makes pfctl -sI to work when more than 64 interfaces/interface groups
> are to be displayed.
Reads fine and works, OK kn.
Limiting output to specific groups/interfaces keeps working as well:
# ./pfctl -sI -i g64g
g64g
vio0
>
> In order to test diff below I create 64 groups for vio0 interface:
>
> for i in `seq 64` ; do ifconfig vio0 group g$i\g ; done
>
> then I use pfctl -sI to display them. with diff below things do work:
>
> netlock# pfctl -sI|wc -l
> 72
> netlock#
You might as well turn that into a new regress test.
> does diff below work for you too?
> thank you for giving patch below a try.
>
> regards
> sashan
>
> --------8<---------------8<---------------8<------------------8<--------
> diff --git a/sbin/pfctl/pfctl_table.c b/sbin/pfctl/pfctl_table.c
> index 5c0c32e5961..7966fe9ac51 100644
> --- a/sbin/pfctl/pfctl_table.c
> +++ b/sbin/pfctl/pfctl_table.c
> @@ -583,18 +583,16 @@ pfctl_show_ifaces(const char *filter, int opts)
> {
> struct pfr_buffer b;
> struct pfi_kif *p;
> - int i = 0;
>
> bzero(&b, sizeof(b));
> b.pfrb_type = PFRB_IFACES;
> for (;;) {
> - pfr_buf_grow(&b, b.pfrb_size);
> + pfr_buf_grow(&b, 0);
> b.pfrb_size = b.pfrb_msize;
> if (pfi_get_ifaces(filter, b.pfrb_caddr, &b.pfrb_size))
> errx(1, "%s", pf_strerror(errno));
> - if (b.pfrb_size <= b.pfrb_msize)
> + if (b.pfrb_size < b.pfrb_msize)
> break;
> - i++;
> }
> if (opts & PF_OPT_SHOWALL)
> pfctl_print_title("INTERFACES:");
> diff --git a/sys/net/pf_if.c b/sys/net/pf_if.c
> index e23c14e6769..24d37ab4f20 100644
> --- a/sys/net/pf_if.c
> +++ b/sys/net/pf_if.c
> @@ -766,12 +766,13 @@ pfi_get_ifaces(const char *name, struct pfi_kif *buf,
> int *size)
> nextp = RB_NEXT(pfi_ifhead, &pfi_ifs, p);
> if (pfi_skip_if(name, p))
> continue;
> - if (*size > n++) {
> + if (*size > ++n) {
You can save the else and one level of indent by doing
if (*size <= ++n)
break;
...
> if (!p->pfik_tzero)
> p->pfik_tzero = gettime();
> memcpy(buf++, p, sizeof(*buf));
> nextp = RB_NEXT(pfi_ifhead, &pfi_ifs, p);
This duplicate nextp assignment seems useless.
It's already pointing at the next entry as done in the first line of the
for loop...
which might as well be a simpler RB_FOREACH, avoiding nextp completely.
I can send a follow-up diff for that after you fixed it, or we clean out
the unused i and nextp variables and switch to RB_FOREACH first.
As you like.
> - }
> + } else
> + break;
> }
> *size = n;
> }
>
