cksum remove redundant code
Hi,
in_ifcap_cksum() checks ifp == NULL
in_hdr_cksum_out() sets ip_sum = 0
in_proto_cksum_out() and in6_proto_cksum_out() always write
th_sum if M_TCP_CSUM_OUT is set and proto is IPPROTO_TCP.
ok?
bluhm
Index: netinet/ip_output.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/ip_output.c,v
retrieving revision 1.388
diff -u -p -r1.388 ip_output.c
--- netinet/ip_output.c 22 May 2023 16:08:34 - 1.388
+++ netinet/ip_output.c 26 May 2023 11:55:49 -
@@ -1801,7 +1801,7 @@ in_hdr_cksum_out(struct mbuf *m, struct
struct ip *ip = mtod(m, struct ip *);
ip->ip_sum = 0;
- if (ifp && in_ifcap_cksum(m, ifp, IFCAP_CSUM_IPv4)) {
+ if (in_ifcap_cksum(m, ifp, IFCAP_CSUM_IPv4)) {
SET(m->m_pkthdr.csum_flags, M_IPV4_CSUM_OUT);
} else {
ipstat_inc(ips_outswcsum);
Index: netinet/tcp_output.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_output.c,v
retrieving revision 1.138
diff -u -p -r1.138 tcp_output.c
--- netinet/tcp_output.c15 May 2023 16:34:56 - 1.138
+++ netinet/tcp_output.c26 May 2023 15:19:12 -
@@ -1295,7 +1295,6 @@ tcp_chopper(struct mbuf *m0, struct mbuf
/* copy and adjust IP header, calculate checksum */
SET(m->m_pkthdr.csum_flags, M_TCP_CSUM_OUT);
- mhth->th_sum = 0;
if (ip) {
struct ip *mhip;
@@ -1328,10 +1327,8 @@ tcp_chopper(struct mbuf *m0, struct mbuf
}
/* adjust IP header, calculate checksum */
SET(m0->m_pkthdr.csum_flags, M_TCP_CSUM_OUT);
- th->th_sum = 0;
if (ip) {
ip->ip_len = htons(m0->m_pkthdr.len);
- ip->ip_sum = 0;
in_hdr_cksum_out(m0, ifp);
in_proto_cksum_out(m0, ifp);
}
Re: pfioctl: drop net lock from DIOCGETIFACES, DIOC{SET,CLR}IFFLAG
On Fri, May 26, 2023 at 05:28:01PM +0300, Vitaliy Makkoveev wrote: > On Fri, May 26, 2023 at 01:03:13PM +, Klemens Nanni wrote: > > snmpd(8) and 'pfctl -s Interfaces' dump pf's internal list of interfaces. > > > > pf.conf's 'set skip on ifN' and 'pfctl -F all|Reset' set and clear flags, > > PFI_IFLAG_SKIP being the only flag. > > > > (There's no other usage of these ioctls in base.) > > > > pf's internal interface list is completely protected by the pf lock, > > pf lock assertions since pf_if.c r1.110 from over a week ago support this. > > > > OK? > > > > pfi_skip_if() called by pfi_get_ifaces() performs `ifgl_next' list > walkthrough. This list is netlock protected, so the netlock around > pfi_get_ifaces() should be kept, but relaxed to shared netlock. Indeed, thanks. Index: pf_ioctl.c === RCS file: /cvs/src/sys/net/pf_ioctl.c,v retrieving revision 1.405 diff -u -p -r1.405 pf_ioctl.c --- pf_ioctl.c 26 May 2023 12:13:26 - 1.405 +++ pf_ioctl.c 26 May 2023 16:10:26 - @@ -2942,11 +2942,11 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a goto fail; } - NET_LOCK(); + NET_LOCK_SHARED(); PF_LOCK(); pfi_get_ifaces(io->pfiio_name, kif_buf, >pfiio_size); PF_UNLOCK(); - NET_UNLOCK(); + NET_UNLOCK_SHARED(); if (copyout(kif_buf, io->pfiio_buffer, sizeof(*kif_buf) * io->pfiio_size)) error = EFAULT; @@ -2962,11 +2962,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a goto fail; } - NET_LOCK(); PF_LOCK(); error = pfi_set_flags(io->pfiio_name, io->pfiio_flags); PF_UNLOCK(); - NET_UNLOCK(); break; } @@ -2978,11 +2976,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a goto fail; } - NET_LOCK(); PF_LOCK(); error = pfi_clear_flags(io->pfiio_name, io->pfiio_flags); PF_UNLOCK(); - NET_UNLOCK(); break; }
Re: iked: ibuf saga step 2
On Fri, May 26, 2023 at 11:18:26AM +0200, Theo Buehler wrote:
> On Fri, May 26, 2023 at 10:11:32AM +0200, Claudio Jeker wrote:
> > Kill ibuf_prepend() it is used only once and the function does unholy
> > things to the ibuf passed in. Just do the obivous dance in the callee.
> > The only thing to be careful about is the fact that all pointers of buf
> > are replaced (msg->msg_data).
> >
> > Tested with iked -t (which should use this codepath).
>
> What can I say. Unholy is a nice way of putting it. This is rather
> horrible. I agree, inlining it makes sense.
>
> It is a bit scary to do ibuf_free(buf); (which ibuf_prepend() is careful
> not to do) since that is passed in by the caller. In turn, pointers are
> no longer transferred via memcpy, which is a good thing in my book...
This ibuf_prepend() pattern is scary. Now if we have more places where
something like this is used then it may make sense to add an
ibuf_prepend() to the real ibuf API but right now I only found this one
use case.
> Arguably, ownership of the caller's buf is transferred to resp.msg_data
> in all cases, so that is fine except that it is still a bit of a trap.
> If this was my code I would probably insist on invalidating the buf in
> the caller after hanging it onto resp.
The livecycle of these bufs is a bit of a mysterybox. Now the
msg->msg_data buf is created in ikev2_msg_init() and should not have other
pointers lingering around. So I agree with your assesment and that this is
still a bit of a trap.
This codebase is using ibufs in very creative ways and it is not always
clear to me why it is done that way.
> ok tb
>
> But please fix the leak indicated below.
Fixed, thanks for spotting.
> > --
> > :wq Claudio
> >
> > Index: iked.h
> > ===
> > RCS file: /cvs/src/sbin/iked/iked.h,v
> > retrieving revision 1.213
> > diff -u -p -r1.213 iked.h
> > --- iked.h 23 May 2023 13:57:14 - 1.213
> > +++ iked.h 26 May 2023 07:21:43 -
> > @@ -1279,7 +1279,6 @@ struct ibuf *
> > ibuf_dup(struct ibuf *);
> > struct ibuf *
> > ibuf_random(size_t);
> > -int ibuf_prepend(struct ibuf *, void *, size_t);
> > int ibuf_strcat(struct ibuf **, const char *);
> > int ibuf_strlen(struct ibuf *);
> >
> > Index: ikev2_msg.c
> > ===
> > RCS file: /cvs/src/sbin/iked/ikev2_msg.c,v
> > retrieving revision 1.92
> > diff -u -p -r1.92 ikev2_msg.c
> > --- ikev2_msg.c 23 May 2023 13:57:14 - 1.92
> > +++ ikev2_msg.c 26 May 2023 07:20:13 -
> > @@ -290,10 +290,17 @@ ikev2_msg_send(struct iked *env, struct
> > ibuf_length(buf), isnatt ? ", NAT-T" : "");
> >
> > if (isnatt) {
> > - if (ibuf_prepend(buf, , sizeof(natt)) == -1) {
> > + struct ibuf *new;
> > + if ((new = ibuf_new(, sizeof(natt))) == NULL) {
> > log_debug("%s: failed to set NAT-T", __func__);
> > return (-1);
> > }
> > + if (ibuf_cat(new, buf) == -1) {
>
> This needs an
>
> ibuf_free(new);
>
> > + log_debug("%s: failed to set NAT-T", __func__);
> > + return (-1);
> > + }
> > + ibuf_free(buf);
> > + buf = msg->msg_data = new;
> > }
> >
> > if (sendtofrom(msg->msg_fd, ibuf_data(buf), ibuf_size(buf), 0,
> > Index: imsg_util.c
> > ===
> > RCS file: /cvs/src/sbin/iked/imsg_util.c,v
> > retrieving revision 1.16
> > diff -u -p -r1.16 imsg_util.c
> > --- imsg_util.c 23 May 2023 13:57:14 - 1.16
> > +++ imsg_util.c 26 May 2023 07:21:26 -
> > @@ -146,25 +146,6 @@ ibuf_setsize(struct ibuf *buf, size_t le
> > }
> >
> > int
> > -ibuf_prepend(struct ibuf *buf, void *data, size_t len)
> > -{
> > - struct ibuf *new;
> > -
> > - /* Swap buffers (we could also use memmove here) */
> > - if ((new = ibuf_new(data, len)) == NULL)
> > - return (-1);
> > - if (ibuf_cat(new, buf) == -1) {
> > - ibuf_free(new);
> > - return (-1);
> > - }
> > - free(buf->buf);
> > - memcpy(buf, new, sizeof(*buf));
> > - free(new);
> > -
> > - return (0);
> > -}
> > -
> > -int
> > ibuf_strcat(struct ibuf **buf, const char *s)
> > {
> > size_t slen;
> >
>
--
:wq Claudio
Re: Relax netlock to shared netlock and push it down to mrt_sysctl_mfc()
On Wed, May 17, 2023 at 01:02:58PM +0300, Vitaliy Makkoveev wrote:
> mrt_rtwalk_mfcsysctl() performs read-only access to protected data, so
> rtable_walk() could be called with shared netlock.
While I think the NET_LOCK_SHARED() is not sufficent, you can move
the NET_LOCK() into mrt_sysctl_mfc(). Only mrt_rtwalk_mfcsysctl
needs protection.
That diff would be OK bluhm@
> Index: sys/netinet/ip_input.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_input.c,v
> retrieving revision 1.384
> diff -u -p -r1.384 ip_input.c
> --- sys/netinet/ip_input.c16 May 2023 19:36:00 - 1.384
> +++ sys/netinet/ip_input.c17 May 2023 09:59:16 -
> @@ -1712,10 +1712,7 @@ ip_sysctl(int *name, u_int namelen, void
> case IPCTL_MRTMFC:
> if (newp)
> return (EPERM);
> - NET_LOCK();
> - error = mrt_sysctl_mfc(oldp, oldlenp);
> - NET_UNLOCK();
> - return (error);
> + return (mrt_sysctl_mfc(oldp, oldlenp));
> case IPCTL_MRTVIF:
> if (newp)
> return (EPERM);
> Index: sys/netinet/ip_mroute.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_mroute.c,v
> retrieving revision 1.138
> diff -u -p -r1.138 ip_mroute.c
> --- sys/netinet/ip_mroute.c 19 Apr 2023 20:03:51 - 1.138
> +++ sys/netinet/ip_mroute.c 17 May 2023 09:59:16 -
> @@ -479,10 +479,12 @@ mrt_sysctl_mfc(void *oldp, size_t *oldle
> msa.msa_len = *oldlenp;
> msa.msa_needed = 0;
>
> + NET_LOCK_SHARED();
> for (rtableid = 0; rtableid <= RT_TABLEID_MAX; rtableid++) {
> rtable_walk(rtableid, AF_INET, NULL, mrt_rtwalk_mfcsysctl,
> );
> }
> + NET_UNLOCK_SHARED();
>
> if (msa.msa_minfos != NULL && msa.msa_needed > 0 &&
> (error = copyout(msa.msa_minfos, oldp, msa.msa_needed)) != 0) {
Re: Relax netlock to shared netlock and push it down to mrt_sysctl_mfc()
On Fri, May 26, 2023 at 06:25:46PM +0300, Vitaliy Makkoveev wrote:
> On Fri, May 26, 2023 at 05:08:06PM +0200, Alexander Bluhm wrote:
> > On Fri, May 26, 2023 at 05:29:58PM +0300, Vitaliy Makkoveev wrote:
> > > On Wed, May 17, 2023 at 01:02:58PM +0300, Vitaliy Makkoveev wrote:
> > > > mrt_rtwalk_mfcsysctl() performs read-only access to protected data, so
> > > > rtable_walk() could be called with shared netlock.
> > >
> > > Regardless on sysctl(2) unlocking backout, the netlock around
> > > mrt_sysctl_mfc() could be relaxed to shared netlock.
> >
> > IP multicast is far from MP ready. As a usual workaround I use
> > exclusive netlock or shared netlock plus kernel lock.
> >
> > Whenever I have to call something with multicast from a section
> > that has only shared netlock, I grab the kenrel lock.
> >
> > So using only NET_LOCK_SHARED() for reading multicast data does not
> > seem enough.
>
> mrt_rtwalk_mfcsysctl() does read-only access. Since the sysctl(2)
> unlocking was reverted, this will be "shared netlock plus kernel lock"
> case.
>
> > Look at the way ip_input_if() calls ip_mforward(). Maybe we should
> > start making ip_mroute.c MP safe. Unfortunately I have no test
> > environment for that.
>
> mpi@ said the kernel lock removal from uvm_swap_free() will be easy. So
> I want to try to remove it and push sysctl(2) unlocking back.
But you cannot do both. Move to shared lock in mrt_rtwalk_mfcsysctl,
and remove kernel lock from sysctl.
Write access is done with shared netlock plus kernel lock.
ip_input_if() -> ip_mforward() -> mfc_add() -> update_mfc_params() ->
mrt_mcast_del() -> rt->rt_llinfo = NULL;
So shared netlock alone is not sufficient for read access.
The popper way is to add some locking to mroute to protect itself
when running with shared netlock.
bluhm
> > > > Index: sys/netinet/ip_input.c
> > > > ===
> > > > RCS file: /cvs/src/sys/netinet/ip_input.c,v
> > > > retrieving revision 1.384
> > > > diff -u -p -r1.384 ip_input.c
> > > > --- sys/netinet/ip_input.c 16 May 2023 19:36:00 - 1.384
> > > > +++ sys/netinet/ip_input.c 17 May 2023 09:59:16 -
> > > > @@ -1712,10 +1712,7 @@ ip_sysctl(int *name, u_int namelen, void
> > > > case IPCTL_MRTMFC:
> > > > if (newp)
> > > > return (EPERM);
> > > > - NET_LOCK();
> > > > - error = mrt_sysctl_mfc(oldp, oldlenp);
> > > > - NET_UNLOCK();
> > > > - return (error);
> > > > + return (mrt_sysctl_mfc(oldp, oldlenp));
> > > > case IPCTL_MRTVIF:
> > > > if (newp)
> > > > return (EPERM);
> > > > Index: sys/netinet/ip_mroute.c
> > > > ===
> > > > RCS file: /cvs/src/sys/netinet/ip_mroute.c,v
> > > > retrieving revision 1.138
> > > > diff -u -p -r1.138 ip_mroute.c
> > > > --- sys/netinet/ip_mroute.c 19 Apr 2023 20:03:51 - 1.138
> > > > +++ sys/netinet/ip_mroute.c 17 May 2023 09:59:16 -
> > > > @@ -479,10 +479,12 @@ mrt_sysctl_mfc(void *oldp, size_t *oldle
> > > > msa.msa_len = *oldlenp;
> > > > msa.msa_needed = 0;
> > > >
> > > > + NET_LOCK_SHARED();
> > > > for (rtableid = 0; rtableid <= RT_TABLEID_MAX; rtableid++) {
> > > > rtable_walk(rtableid, AF_INET, NULL,
> > > > mrt_rtwalk_mfcsysctl,
> > > > );
> > > > }
> > > > + NET_UNLOCK_SHARED();
> > > >
> > > > if (msa.msa_minfos != NULL && msa.msa_needed > 0 &&
> > > > (error = copyout(msa.msa_minfos, oldp, msa.msa_needed)) !=
> > > > 0) {
> > > >
> >
Re: Relax netlock to shared netlock and push it down to mrt_sysctl_mfc()
On Fri, May 26, 2023 at 05:08:06PM +0200, Alexander Bluhm wrote:
> On Fri, May 26, 2023 at 05:29:58PM +0300, Vitaliy Makkoveev wrote:
> > On Wed, May 17, 2023 at 01:02:58PM +0300, Vitaliy Makkoveev wrote:
> > > mrt_rtwalk_mfcsysctl() performs read-only access to protected data, so
> > > rtable_walk() could be called with shared netlock.
> > >
> >
> > Regardless on sysctl(2) unlocking backout, the netlock around
> > mrt_sysctl_mfc() could be relaxed to shared netlock.
>
> IP multicast is far from MP ready. As a usual workaround I use
> exclusive netlock or shared netlock plus kernel lock.
>
> Whenever I have to call something with multicast from a section
> that has only shared netlock, I grab the kenrel lock.
>
> So using only NET_LOCK_SHARED() for reading multicast data does not
> seem enough.
>
mrt_rtwalk_mfcsysctl() does read-only access. Since the sysctl(2)
unlocking was reverted, this will be "shared netlock plus kernel lock"
case.
> Look at the way ip_input_if() calls ip_mforward(). Maybe we should
> start making ip_mroute.c MP safe. Unfortunately I have no test
> environment for that.
>
mpi@ said the kernel lock removal from uvm_swap_free() will be easy. So
I want to try to remove it and push sysctl(2) unlocking back.
> bluhm
>
> > > Index: sys/netinet/ip_input.c
> > > ===
> > > RCS file: /cvs/src/sys/netinet/ip_input.c,v
> > > retrieving revision 1.384
> > > diff -u -p -r1.384 ip_input.c
> > > --- sys/netinet/ip_input.c16 May 2023 19:36:00 - 1.384
> > > +++ sys/netinet/ip_input.c17 May 2023 09:59:16 -
> > > @@ -1712,10 +1712,7 @@ ip_sysctl(int *name, u_int namelen, void
> > > case IPCTL_MRTMFC:
> > > if (newp)
> > > return (EPERM);
> > > - NET_LOCK();
> > > - error = mrt_sysctl_mfc(oldp, oldlenp);
> > > - NET_UNLOCK();
> > > - return (error);
> > > + return (mrt_sysctl_mfc(oldp, oldlenp));
> > > case IPCTL_MRTVIF:
> > > if (newp)
> > > return (EPERM);
> > > Index: sys/netinet/ip_mroute.c
> > > ===
> > > RCS file: /cvs/src/sys/netinet/ip_mroute.c,v
> > > retrieving revision 1.138
> > > diff -u -p -r1.138 ip_mroute.c
> > > --- sys/netinet/ip_mroute.c 19 Apr 2023 20:03:51 - 1.138
> > > +++ sys/netinet/ip_mroute.c 17 May 2023 09:59:16 -
> > > @@ -479,10 +479,12 @@ mrt_sysctl_mfc(void *oldp, size_t *oldle
> > > msa.msa_len = *oldlenp;
> > > msa.msa_needed = 0;
> > >
> > > + NET_LOCK_SHARED();
> > > for (rtableid = 0; rtableid <= RT_TABLEID_MAX; rtableid++) {
> > > rtable_walk(rtableid, AF_INET, NULL, mrt_rtwalk_mfcsysctl,
> > > );
> > > }
> > > + NET_UNLOCK_SHARED();
> > >
> > > if (msa.msa_minfos != NULL && msa.msa_needed > 0 &&
> > > (error = copyout(msa.msa_minfos, oldp, msa.msa_needed)) != 0) {
> > >
>
Re: Relax netlock to shared netlock and push it down to mrt_sysctl_mfc()
On Fri, May 26, 2023 at 05:29:58PM +0300, Vitaliy Makkoveev wrote:
> On Wed, May 17, 2023 at 01:02:58PM +0300, Vitaliy Makkoveev wrote:
> > mrt_rtwalk_mfcsysctl() performs read-only access to protected data, so
> > rtable_walk() could be called with shared netlock.
> >
>
> Regardless on sysctl(2) unlocking backout, the netlock around
> mrt_sysctl_mfc() could be relaxed to shared netlock.
IP multicast is far from MP ready. As a usual workaround I use
exclusive netlock or shared netlock plus kernel lock.
Whenever I have to call something with multicast from a section
that has only shared netlock, I grab the kenrel lock.
So using only NET_LOCK_SHARED() for reading multicast data does not
seem enough.
Look at the way ip_input_if() calls ip_mforward(). Maybe we should
start making ip_mroute.c MP safe. Unfortunately I have no test
environment for that.
bluhm
> > Index: sys/netinet/ip_input.c
> > ===
> > RCS file: /cvs/src/sys/netinet/ip_input.c,v
> > retrieving revision 1.384
> > diff -u -p -r1.384 ip_input.c
> > --- sys/netinet/ip_input.c 16 May 2023 19:36:00 - 1.384
> > +++ sys/netinet/ip_input.c 17 May 2023 09:59:16 -
> > @@ -1712,10 +1712,7 @@ ip_sysctl(int *name, u_int namelen, void
> > case IPCTL_MRTMFC:
> > if (newp)
> > return (EPERM);
> > - NET_LOCK();
> > - error = mrt_sysctl_mfc(oldp, oldlenp);
> > - NET_UNLOCK();
> > - return (error);
> > + return (mrt_sysctl_mfc(oldp, oldlenp));
> > case IPCTL_MRTVIF:
> > if (newp)
> > return (EPERM);
> > Index: sys/netinet/ip_mroute.c
> > ===
> > RCS file: /cvs/src/sys/netinet/ip_mroute.c,v
> > retrieving revision 1.138
> > diff -u -p -r1.138 ip_mroute.c
> > --- sys/netinet/ip_mroute.c 19 Apr 2023 20:03:51 - 1.138
> > +++ sys/netinet/ip_mroute.c 17 May 2023 09:59:16 -
> > @@ -479,10 +479,12 @@ mrt_sysctl_mfc(void *oldp, size_t *oldle
> > msa.msa_len = *oldlenp;
> > msa.msa_needed = 0;
> >
> > + NET_LOCK_SHARED();
> > for (rtableid = 0; rtableid <= RT_TABLEID_MAX; rtableid++) {
> > rtable_walk(rtableid, AF_INET, NULL, mrt_rtwalk_mfcsysctl,
> > );
> > }
> > + NET_UNLOCK_SHARED();
> >
> > if (msa.msa_minfos != NULL && msa.msa_needed > 0 &&
> > (error = copyout(msa.msa_minfos, oldp, msa.msa_needed)) != 0) {
> >
Re: Relax netlock to shared netlock and push it down to mrt_sysctl_vif()
On Wed, May 17, 2023 at 01:08:52PM +0300, Vitaliy Makkoveev wrote:
> Also read-only access to netlock protected data.
>
Regardless on sysctl(2) unlocking backout, the netlock around
mrt_sysctl_vif() could be relaxed to shared netlock.
> Index: sys/netinet/ip_input.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_input.c,v
> retrieving revision 1.384
> diff -u -p -r1.384 ip_input.c
> --- sys/netinet/ip_input.c16 May 2023 19:36:00 - 1.384
> +++ sys/netinet/ip_input.c17 May 2023 10:06:49 -
> @@ -1719,10 +1719,7 @@ ip_sysctl(int *name, u_int namelen, void
> case IPCTL_MRTVIF:
> if (newp)
> return (EPERM);
> - NET_LOCK();
> - error = mrt_sysctl_vif(oldp, oldlenp);
> - NET_UNLOCK();
> - return (error);
> + return (mrt_sysctl_vif(oldp, oldlenp));
> #else
> case IPCTL_MRTPROTO:
> case IPCTL_MRTSTATS:
> Index: sys/netinet/ip_mroute.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_mroute.c,v
> retrieving revision 1.138
> diff -u -p -r1.138 ip_mroute.c
> --- sys/netinet/ip_mroute.c 19 Apr 2023 20:03:51 - 1.138
> +++ sys/netinet/ip_mroute.c 17 May 2023 10:06:49 -
> @@ -356,6 +356,8 @@ mrt_sysctl_vif(void *oldp, size_t *oldle
> given = *oldlenp;
> needed = 0;
> memset(, 0, sizeof vinfo);
> +
> + NET_LOCK_SHARED();
> TAILQ_FOREACH(ifp, , if_list) {
> if ((vifp = (struct vif *)ifp->if_mcast) == NULL)
> continue;
> @@ -375,11 +377,15 @@ mrt_sysctl_vif(void *oldp, size_t *oldle
> int error;
>
> error = copyout(, where, sizeof(vinfo));
> - if (error)
> + if (error) {
> + NET_UNLOCK_SHARED();
> return (error);
> + }
> where += sizeof(vinfo);
> }
> }
> + NET_UNLOCK_SHARED();
> +
> if (where) {
> *oldlenp = needed;
> if (given < needed)
>
Re: Relax netlock to shared netlock and push it down to mrt_sysctl_mfc()
On Wed, May 17, 2023 at 01:02:58PM +0300, Vitaliy Makkoveev wrote:
> mrt_rtwalk_mfcsysctl() performs read-only access to protected data, so
> rtable_walk() could be called with shared netlock.
>
Regardless on sysctl(2) unlocking backout, the netlock around
mrt_sysctl_mfc() could be relaxed to shared netlock.
> Index: sys/netinet/ip_input.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_input.c,v
> retrieving revision 1.384
> diff -u -p -r1.384 ip_input.c
> --- sys/netinet/ip_input.c16 May 2023 19:36:00 - 1.384
> +++ sys/netinet/ip_input.c17 May 2023 09:59:16 -
> @@ -1712,10 +1712,7 @@ ip_sysctl(int *name, u_int namelen, void
> case IPCTL_MRTMFC:
> if (newp)
> return (EPERM);
> - NET_LOCK();
> - error = mrt_sysctl_mfc(oldp, oldlenp);
> - NET_UNLOCK();
> - return (error);
> + return (mrt_sysctl_mfc(oldp, oldlenp));
> case IPCTL_MRTVIF:
> if (newp)
> return (EPERM);
> Index: sys/netinet/ip_mroute.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_mroute.c,v
> retrieving revision 1.138
> diff -u -p -r1.138 ip_mroute.c
> --- sys/netinet/ip_mroute.c 19 Apr 2023 20:03:51 - 1.138
> +++ sys/netinet/ip_mroute.c 17 May 2023 09:59:16 -
> @@ -479,10 +479,12 @@ mrt_sysctl_mfc(void *oldp, size_t *oldle
> msa.msa_len = *oldlenp;
> msa.msa_needed = 0;
>
> + NET_LOCK_SHARED();
> for (rtableid = 0; rtableid <= RT_TABLEID_MAX; rtableid++) {
> rtable_walk(rtableid, AF_INET, NULL, mrt_rtwalk_mfcsysctl,
> );
> }
> + NET_UNLOCK_SHARED();
>
> if (msa.msa_minfos != NULL && msa.msa_needed > 0 &&
> (error = copyout(msa.msa_minfos, oldp, msa.msa_needed)) != 0) {
>
Re: pfioctl: drop net lock from DIOCGETIFACES, DIOC{SET,CLR}IFFLAG
On Fri, May 26, 2023 at 01:03:13PM +, Klemens Nanni wrote: > snmpd(8) and 'pfctl -s Interfaces' dump pf's internal list of interfaces. > > pf.conf's 'set skip on ifN' and 'pfctl -F all|Reset' set and clear flags, > PFI_IFLAG_SKIP being the only flag. > > (There's no other usage of these ioctls in base.) > > pf's internal interface list is completely protected by the pf lock, > pf lock assertions since pf_if.c r1.110 from over a week ago support this. > > OK? > pfi_skip_if() called by pfi_get_ifaces() performs `ifgl_next' list walkthrough. This list is netlock protected, so the netlock around pfi_get_ifaces() should be kept, but relaxed to shared netlock. > Index: pf_ioctl.c > === > RCS file: /cvs/src/sys/net/pf_ioctl.c,v > retrieving revision 1.405 > diff -u -p -r1.405 pf_ioctl.c > --- pf_ioctl.c26 May 2023 12:13:26 - 1.405 > +++ pf_ioctl.c26 May 2023 12:46:37 - > @@ -2942,11 +2942,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a > goto fail; > } > > - NET_LOCK(); > PF_LOCK(); > pfi_get_ifaces(io->pfiio_name, kif_buf, >pfiio_size); > PF_UNLOCK(); > - NET_UNLOCK(); > if (copyout(kif_buf, io->pfiio_buffer, sizeof(*kif_buf) * > io->pfiio_size)) > error = EFAULT; > @@ -2962,11 +2960,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a > goto fail; > } > > - NET_LOCK(); > PF_LOCK(); > error = pfi_set_flags(io->pfiio_name, io->pfiio_flags); > PF_UNLOCK(); > - NET_UNLOCK(); > break; > } > > @@ -2978,11 +2974,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a > goto fail; > } > > - NET_LOCK(); > PF_LOCK(); > error = pfi_clear_flags(io->pfiio_name, io->pfiio_flags); > PF_UNLOCK(); > - NET_UNLOCK(); > break; > } > >
Re: Virtio fix for testing
On Wed, May 24, 2023 at 08:50:26PM +0200, Stefan Fritsch wrote: > I forgot to mention that no stress test is necessary. If it boots and the > virtio devices work at all, that should be enough. Works for me on Linux/KVM with the following devices: vga1 at pci0 dev 2 function 0 "Qumranet Virtio 1.x GPU" rev 0x01 virtio0 at pci0 dev 4 function 0 "Qumranet Virtio Storage" rev 0x00 virtio1 at pci0 dev 6 function 0 "Qumranet Virtio Console" rev 0x00 virtio2 at pci0 dev 7 function 0 "Qumranet Virtio Memory Balloon" rev 0x00 virtio3 at pci0 dev 8 function 0 "Qumranet Virtio Network" rev 0x00 and on OpenBSD/VMM with: virtio0 at pci0 dev 1 function 0 "Qumranet Virtio RNG" rev 0x00 virtio1 at pci0 dev 2 function 0 "Qumranet Virtio Network" rev 0x00 virtio2 at pci0 dev 3 function 0 "Qumranet Virtio Storage" rev 0x00 virtio3 at pci0 dev 4 function 0 "Qumranet Virtio SCSI" rev 0x00 Thanks, Jan
ospf6d fib reload [Re: bgpd fix for possible crash in SE]
On 2023/05/26 13:52, Stuart Henderson wrote:
> I think my main issues come around LS_REFRESH_TIME intervals, when
> there's loads of churn and "ospf6d: ospf engine" can be busy for
> minutes at a time (not always, but very often). Don't know if that rings
> any bells for anyone... (I am now reminded that RTM_DESYNC isn't handled
> by ospf6d which probably doesn't help matters).
Here's a first attempt at porting the fib reload/desync diffs from
ospfd to ospf6d ... Not sure if it's good yet, but it didn't immediately
crash and burn when I ran "ospf6ctl fib reload", at least.
Index: ospf6ctl/ospf6ctl.8
===
RCS file: /cvs/src/usr.sbin/ospf6ctl/ospf6ctl.8,v
retrieving revision 1.13
diff -u -p -r1.13 ospf6ctl.8
--- ospf6ctl/ospf6ctl.8 2 Mar 2023 17:09:53 - 1.13
+++ ospf6ctl/ospf6ctl.8 26 May 2023 13:37:55 -
@@ -58,6 +58,9 @@ Remove the learned routes from the FIB.
Decoupling the FIB from an OSPF router may create routing loops and could cause
major routing issues in the complete OSPF cloud.
Only routers with just one link to the OSPF cloud can safely decouple the FIB.
+.It Cm fib reload
+Refetches and relearns the routes in the Forwarding Information Base
+a.k.a. the kernel routing table.
.It Cm log brief
Disable verbose debug logging.
.It Cm log verbose
Index: ospf6ctl/ospf6ctl.c
===
RCS file: /cvs/src/usr.sbin/ospf6ctl/ospf6ctl.c,v
retrieving revision 1.53
diff -u -p -r1.53 ospf6ctl.c
--- ospf6ctl/ospf6ctl.c 27 Dec 2022 12:11:39 - 1.53
+++ ospf6ctl/ospf6ctl.c 26 May 2023 13:37:55 -
@@ -225,6 +225,11 @@ main(int argc, char *argv[])
printf("decouple request sent.\n");
done = 1;
break;
+ case FIB_RELOAD:
+ imsg_compose(ibuf, IMSG_CTL_FIB_RELOAD, 0, 0, -1, NULL, 0);
+ printf("reload request sent.\n");
+ done = 1;
+ break;
case LOG_VERBOSE:
verbose = 1;
/* FALLTHROUGH */
@@ -304,6 +309,7 @@ main(int argc, char *argv[])
case FIB:
case FIB_COUPLE:
case FIB_DECOUPLE:
+ case FIB_RELOAD:
case LOG_VERBOSE:
case LOG_BRIEF:
case RELOAD:
Index: ospf6ctl/parser.c
===
RCS file: /cvs/src/usr.sbin/ospf6ctl/parser.c,v
retrieving revision 1.14
diff -u -p -r1.14 parser.c
--- ospf6ctl/parser.c 26 May 2019 09:27:09 - 1.14
+++ ospf6ctl/parser.c 26 May 2023 13:37:55 -
@@ -73,6 +73,7 @@ static const struct token t_main[] = {
static const struct token t_fib[] = {
{ KEYWORD, "couple", FIB_COUPLE, NULL},
{ KEYWORD, "decouple", FIB_DECOUPLE, NULL},
+ { KEYWORD, "reload", FIB_RELOAD, NULL},
{ ENDTOKEN, "", NONE, NULL}
};
Index: ospf6ctl/parser.h
===
RCS file: /cvs/src/usr.sbin/ospf6ctl/parser.h,v
retrieving revision 1.9
diff -u -p -r1.9 parser.h
--- ospf6ctl/parser.h 26 May 2019 09:27:09 - 1.9
+++ ospf6ctl/parser.h 26 May 2023 13:37:55 -
@@ -29,6 +29,7 @@ enum actions {
FIB,
FIB_COUPLE,
FIB_DECOUPLE,
+ FIB_RELOAD,
LOG_VERBOSE,
LOG_BRIEF,
SHOW,
Index: ospf6d/control.c
===
RCS file: /cvs/src/usr.sbin/ospf6d/control.c,v
retrieving revision 1.31
diff -u -p -r1.31 control.c
--- ospf6d/control.c8 Mar 2023 04:43:14 - 1.31
+++ ospf6d/control.c26 May 2023 13:37:55 -
@@ -279,6 +279,7 @@ control_dispatch_imsg(int fd, short even
case IMSG_CTL_FIB_DECOUPLE:
ospfe_fib_update(imsg.hdr.type);
/* FALLTHROUGH */
+ case IMSG_CTL_FIB_RELOAD:
case IMSG_CTL_RELOAD:
c->iev.ibuf.pid = imsg.hdr.pid;
ospfe_imsg_compose_parent(imsg.hdr.type, 0, NULL, 0);
Index: ospf6d/kroute.c
===
RCS file: /cvs/src/usr.sbin/ospf6d/kroute.c,v
retrieving revision 1.67
diff -u -p -r1.67 kroute.c
--- ospf6d/kroute.c 8 Mar 2023 04:43:14 - 1.67
+++ ospf6d/kroute.c 26 May 2023 13:37:55 -
@@ -45,16 +45,22 @@ struct {
u_int32_t rtseq;
pid_t pid;
int fib_sync;
+ int fib_serial;
u_int8_tfib_prio;
int fd;
- struct eventev;
+ struct eventev, reload;
u_int rdomain;
+#define KR_RELOAD_IDLE 0
OpenBSD Errata: May 26, 2023 (rpki ssl)
Errata patches for rpki-client and LibreSSL libssl have been released for OpenBSD 7.2 and 7.3. Binary updates for the amd64, i386 and arm64 platform are available via the syspatch utility. Source code patches can be found on the respective errata page: https://www.openbsd.org/errata72.html https://www.openbsd.org/errata73.html
pfioctl: drop net lock from DIOCGETIFACES, DIOC{SET,CLR}IFFLAG
snmpd(8) and 'pfctl -s Interfaces' dump pf's internal list of interfaces. pf.conf's 'set skip on ifN' and 'pfctl -F all|Reset' set and clear flags, PFI_IFLAG_SKIP being the only flag. (There's no other usage of these ioctls in base.) pf's internal interface list is completely protected by the pf lock, pf lock assertions since pf_if.c r1.110 from over a week ago support this. OK? Index: pf_ioctl.c === RCS file: /cvs/src/sys/net/pf_ioctl.c,v retrieving revision 1.405 diff -u -p -r1.405 pf_ioctl.c --- pf_ioctl.c 26 May 2023 12:13:26 - 1.405 +++ pf_ioctl.c 26 May 2023 12:46:37 - @@ -2942,11 +2942,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a goto fail; } - NET_LOCK(); PF_LOCK(); pfi_get_ifaces(io->pfiio_name, kif_buf, >pfiio_size); PF_UNLOCK(); - NET_UNLOCK(); if (copyout(kif_buf, io->pfiio_buffer, sizeof(*kif_buf) * io->pfiio_size)) error = EFAULT; @@ -2962,11 +2960,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a goto fail; } - NET_LOCK(); PF_LOCK(); error = pfi_set_flags(io->pfiio_name, io->pfiio_flags); PF_UNLOCK(); - NET_UNLOCK(); break; } @@ -2978,11 +2974,9 @@ pfioctl(dev_t dev, u_long cmd, caddr_t a goto fail; } - NET_LOCK(); PF_LOCK(); error = pfi_clear_flags(io->pfiio_name, io->pfiio_flags); PF_UNLOCK(); - NET_UNLOCK(); break; }
Re: bgpd fix for possible crash in SE
On 2023/05/25 16:15, Claudio Jeker wrote: > On Thu, May 25, 2023 at 02:20:37PM +0100, Stuart Henderson wrote: > > On 2023/05/25 15:06, Claudio Jeker wrote: > > > sthen@ reported a bgpd SE crash to me and after inspection of the report > > > it looks like he managed to trigger a mistake in session_process_msg(). > > > When for example a NOTIFICATION message is received then the state change > > > clears the rbuf. Now normally the for loop starts over afterwards and the > > > if (p->rbuf == NULL) at the top causes the function to return. > > > In his case the peer had enough messages queued that the if (++processed > > > > MSG_PROCESS_LIMIT) limit triggered after the NOTIFICATION was processed. > > > This hits a break from the for loop and the code at the end of the > > > function adjusting the rbuf trips over the NULL rbuf pointer. > > > > > > This can be fixed in many ways, I decided to just check at the end of the > > > loop that rbuf is still valid. > > > > Thanks for looking into this. OK. > > > > > Triggering this bug is not trivial so it is hard test but the problem is > > > obvious. > > > > indeed, I don't think I have hit this one at all before. > > > > Running sessions over routes maintained by ospf6d seems a fairly > > good way to trigger a number of issues ;) > > I do run ospf6d on some systems but it seems my setup is too small to > trigger all those strange issues. :( Somehow I hit that same SE crash twice more overnight, I don't think I've spotted that one before! I have now updated a handful of routers (some to 7.1 or 2022/06/13 i.e. pre-kroute-changes plus your diff, some to -current) all built with debug symbols (I don't usually get even function names from bgpd cores without symbols). I think my main issues come around LS_REFRESH_TIME intervals, when there's loads of churn and "ospf6d: ospf engine" can be busy for minutes at a time (not always, but very often). Don't know if that rings any bells for anyone... (I am now reminded that RTM_DESYNC isn't handled by ospf6d which probably doesn't help matters). With log verbose, the fsm log messages look like this. 2023-05-26T10:30:34.648Z ospf6d[21331]: if_fsm: event NEIGHBORCHANGE resulted in action ELECT and changing state for interface vlan760 from BCKUP to DR 2023-05-26T10:30:34.648Z ospf6d[21331]: nbr_fsm: event 1_WAY_RECEIVED resulted in action CLEAR_LISTS and changing state for neighbor ID xx.13 (vlan760) from FULL to INIT 2023-05-26T10:30:34.651Z ospf6d[21331]: if_fsm: event NEIGHBORCHANGE resulted in action ELECT and changing state for interface vlan760 from DR to DR 2023-05-26T10:30:34.660Z ospf6d[21331]: nbr_fsm: event 2_WAY_RECEIVED resulted in action EVAL and changing state for neighbor ID xx.13 (vlan760) from INIT to EXSTA 2023-05-26T10:30:34.660Z ospf6d[21331]: if_fsm: event NEIGHBORCHANGE resulted in action ELECT and changing state for interface vlan760 from DR to OTHER 2023-05-26T10:30:34.660Z ospf6d[21331]: nbr_fsm: event NEGOTIATION_DONE resulted in action SNAPSHOT and changing state for neighbor ID xx.13 (vlan760) from EXSTA to SNAP 2023-05-26T10:30:35.145Z ospf6d[21331]: nbr_fsm: event SNAPSHOT_DONE resulted in action SNAPSHOT_DONE and changing state for neighbor ID xx.13 (vlan760) from SNAP to EXCHG 2023-05-26T10:30:42.642Z ospf6d[21331]: nbr_fsm: event EXCHANGE_DONE resulted in action EXCHANGE_DONE and changing state for neighbor ID xx.13 (vlan760) from EXCHG to LOAD 2023-05-26T10:30:55.520Z ospf6d[21331]: nbr_fsm: event LOADING_DONE resulted in action NOTHING and changing state for neighbor ID xx.13 (vlan760) from LOAD to FULL 2023-05-26T10:30:56.911Z ospf6d[21331]: if_fsm: event NEIGHBORCHANGE resulted in action ELECT and changing state for interface vlan761 from DR to DR 2023-05-26T10:30:56.911Z ospf6d[21331]: nbr_fsm: event 1_WAY_RECEIVED resulted in action CLEAR_LISTS and changing state for neighbor ID xx.7 (vlan761) from FULL to INIT 2023-05-26T10:30:58.075Z ospf6d[21331]: nbr_fsm: event 2_WAY_RECEIVED resulted in action EVAL and changing state for neighbor ID xx.7 (vlan761) from INIT to EXSTA 2023-05-26T10:30:58.075Z ospf6d[21331]: if_fsm: event NEIGHBORCHANGE resulted in action ELECT and changing state for interface vlan761 from DR to DR 2023-05-26T10:30:58.142Z ospf6d[21331]: nbr_fsm: event NEGOTIATION_DONE resulted in action SNAPSHOT and changing state for neighbor ID xx.7 (vlan761) from EXSTA to SNAP 2023-05-26T10:30:58.195Z ospf6d[21331]: nbr_fsm: event SNAPSHOT_DONE resulted in action SNAPSHOT_DONE and changing state for neighbor ID xx.7 (vlan761) from SNAP to EXCHG 2023-05-26T10:31:24.853Z ospf6d[21331]: nbr_fsm: event EXCHANGE_DONE resulted in action EXCHANGE_DONE and changing state for neighbor ID xx.7 (vlan761) from EXCHG to FULL 2023-05-26T11:00:32.724Z ospf6d[21331]: if_fsm: event NEIGHBORCHANGE resulted in action ELECT and changing state for interface vlan760
Re: Status of Virtual Function driver for Intel 82599 series port?
My previous patch is partly rejected for OpenBSD current. Because ixv(4) code
depends on ix(4) that has changed to supported TSO/LRO. I rebased my patch for
OpenBSD current. See the patch at the end of this e-mail.
Thank you, Paul B. Henson! He tested my patch on Linux Qemu and now we have
the knowledge to operate the ixv(4). I will write down the following.
Known Issues and Requirements:
1. Do not use 'ifconfig lladdr' on ESXi.
Changing link local address is not permitted on ESXi. If it is changed,
the interface cannot be usable any more. Need to reboot the VM.
Link local address can be changed by ESXi user interface.
On Linux qemu, 'ifconfig lladdr' works.
2. "pc-q35" emulation is required on Linux Qemu.
The default chipset emulation "pc-i440fx" doesn't support MSI-X interrupt.
Ixv(4) requires MSI-X and never work with MSI. So, ixv(4) fails to attach
with "pc-i440fx" emulation.
3. Linux ixgbe driver shows "Unhandled Msg 0010" in dmesg.
Older version of Linux doesn't support GET_LINK_STATE message. Ixv(4)
cannot see physical link state is changed. I see GET_LINK_STATE message
has supported by following commit in Linus repository. But I'm not sure
which distro has this patch.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=366fd1000995d4cf64e1a61a0d78a051550b9841
4. Make sure PF (Primary Function) interface is up.
While PF interface is down, PF driver stop working and doesn't respond to
VF (Virtual Function). Ixv interface cannot be brought up.
5. Linux kernel shows all reset messages from VF in dmesg.
This is not an error case. Ixv(4) sends reset messages when attaching
device and the interface is brought up.
6. Performance
Recent OpenBSD ix(4) supports TSO. It also improves ixv(4) packet
transmission performance significantly. TSO is enabled by default. But
LRO is not supported by VF. There is no way to use LRO in ixv(4).
Increasing MTU size gives you a chance to improve performance but make
sure all intermediate routes can handle the MTU size.
diff --git a/sys/arch/amd64/conf/GENERIC b/sys/arch/amd64/conf/GENERIC
index c8e4ec8284e..2ad357f9c1b 100644
--- a/sys/arch/amd64/conf/GENERIC
+++ b/sys/arch/amd64/conf/GENERIC
@@ -524,6 +524,7 @@ msk*at mskc?# each port of
above
em*at pci? # Intel Pro/1000 ethernet
ixgb* at pci? # Intel Pro/10Gb ethernet
ix*at pci? # Intel 82598EB 10Gb ethernet
+ixv* at pci? # Virtual Function of Intel 82598EB
myx* at pci? # Myricom Myri-10G 10Gb ethernet
oce* at pci? # Emulex OneConnect 10Gb ethernet
txp* at pci? # 3com 3CR990
diff --git a/sys/dev/pci/files.pci b/sys/dev/pci/files.pci
index 101ed502e76..72c8b485938 100644
--- a/sys/dev/pci/files.pci
+++ b/sys/dev/pci/files.pci
@@ -350,13 +350,19 @@ file dev/pci/ixgb_hw.c ixgb
# Intel 82598 10GbE
device ix: ether, ifnet, ifmedia, intrmap, stoeplitz
attach ix at pci
-file dev/pci/if_ix.c ix
-file dev/pci/ixgbe.c ix
-file dev/pci/ixgbe_82598.c ix
-file dev/pci/ixgbe_82599.c ix
-file dev/pci/ixgbe_x540.cix
-file dev/pci/ixgbe_x550.cix
-file dev/pci/ixgbe_phy.c ix
+file dev/pci/if_ix.c ix | ixv
+file dev/pci/ixgbe.c ix | ixv
+file dev/pci/ixgbe_82598.c ix | ixv
+file dev/pci/ixgbe_82599.c ix | ixv
+file dev/pci/ixgbe_x540.cix | ixv
+file dev/pci/ixgbe_x550.cix | ixv
+file dev/pci/ixgbe_phy.c ix | ixv
+
+# Virtual Function of i82599.
+device ixv: ether, ifnet, ifmedia, intrmap, stoeplitz
+attach ixv at pci
+file dev/pci/if_ixv.cixv
+file dev/pci/ixgbe_vf.c ixv
# Intel Ethernet 700 Series
device ixl: ether, ifnet, ifmedia, intrmap, stoeplitz
diff --git a/sys/dev/pci/if_ix.c b/sys/dev/pci/if_ix.c
index 98815b51d62..56d8e305dec 100644
--- a/sys/dev/pci/if_ix.c
+++ b/sys/dev/pci/if_ix.c
@@ -507,7 +507,7 @@ ixgbe_start(struct ifqueue *ifq)
* hardware that this frame is available to transmit.
*/
if (post)
- IXGBE_WRITE_REG(>hw, IXGBE_TDT(txr->me),
+ IXGBE_WRITE_REG(>hw, txr->tail,
txr->next_avail_desc);
}
@@ -705,7 +705,7 @@ ixgbe_watchdog(struct ifnet * ifp)
for (i = 0; i < sc->num_queues; i++, txr++) {
printf("%s: Queue(%d) tdh = %d, hw tdt = %d\n", ifp->if_xname,
i,
IXGBE_READ_REG(hw, IXGBE_TDH(i)),
- IXGBE_READ_REG(hw, IXGBE_TDT(i)));
+ IXGBE_READ_REG(hw, sc->tx_rings[i].tail));
printf("%s: TX(%d) Next TX to Clean =
Re: pkg-config tweaks
There was a small typo which broke xenocara, as noticed by tb@
(sidenote: I hate this shitty configure stuff that can't even give you
the warning messages from tools that ran. *OF COURSE* it's because so
much of that shit talks incessantly even when things are fine)
Index: pkg-config
===
RCS file: /cvs/src/usr.bin/pkg-config/pkg-config,v
retrieving revision 1.95
diff -u -p -r1.95 pkg-config
--- pkg-config 15 Sep 2020 07:18:45 - 1.95
+++ pkg-config 26 May 2023 09:40:46 -
@@ -16,14 +16,20 @@
# ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
# OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
-use strict;
-use warnings;
+use v5.36;
use Config;
use Getopt::Long;
use File::Basename;
use File::stat;
use OpenBSD::PkgConfig;
+use constant {
+ ONLY_I => 1,
+ ONLY_l => 2,
+ ONLY_L => 4,
+ ONLY_OTHER => 8
+};
+
my @PKGPATH = qw(/usr/lib/pkgconfig
/usr/local/lib/pkgconfig
/usr/local/share/pkgconfig
@@ -70,7 +76,7 @@ defined $ENV{PKG_CONFIG_DEBUG_SPEW} ? $m
if ($logfile) {
open my $L, ">>" , $logfile or die;
- print $L beautify_list($0, @ARGV), "\n";
+ say $L beautify_list($0, @ARGV);
close $L;
}
@@ -87,7 +93,7 @@ GetOptions( 'debug' => \$mode{debug},
'help' => \, #does not return
'usage' => \, #does not return
'list-all' => \$mode{list},
- 'version' => sub { print "$version\n" ; exit(0);}
,
+ 'version' => sub { say $version ; exit(0);} ,
'errors-to-stdout' => sub { $mode{estdout} = 1},
'print-errors' => sub { $mode{printerr} = 1},
'silence-errors'=> sub { $mode{printerr} = 0},
@@ -97,13 +103,13 @@ GetOptions('debug' =>
\$mode{debug},
'print-requires'=> \$mode{printrequires},
'print-requires-private' => \$mode{printrequiresprivate},
- 'cflags'=> sub { $mode{cflags} = 3},
- 'cflags-only-I' => sub { $mode{cflags} |= 1},
- 'cflags-only-other' => sub { $mode{cflags} |= 2},
- 'libs' => sub { $mode{libs} = 7},
- 'libs-only-l' => sub { $mode{libs} |= 1},
- 'libs-only-L' => sub { $mode{libs} |= 2},
- 'libs-only-other' => sub { $mode{libs} |= 4},
+ 'cflags'=> sub { $mode{cflags} =
ONLY_I|ONLY_OTHER},
+ 'cflags-only-I' => sub { $mode{cflags} |= ONLY_I},
+ 'cflags-only-other' => sub { $mode{cflags} |= ONLY_OTHER},
+ 'libs' => sub { $mode{libs} =
ONLY_L|ONLY_l|ONLY_OTHER},
+ 'libs-only-l' => sub { $mode{libs} |= ONLY_l},
+ 'libs-only-L' => sub { $mode{libs} |= ONLY_L},
+ 'libs-only-other' => sub { $mode{libs} |= ONLY_OTHER},
'exists'=> sub { $mode{exists} = 1} ,
'validate' => sub { $mode{validate} = 1},
'static'=> sub { $mode{static} = 1},
@@ -178,9 +184,9 @@ sub get_next_module
if ($module =~ m/,/) {
my @ms = split(/,/, $module);
$m = shift @ms;
- unshift(@ARGV, @ms) if (scalar(@ms) > 0);
+ unshift(@ARGV, @ms) if @ms != 0;
} else {
- return $module;
+ return $module;
}
return $m;
@@ -267,16 +273,15 @@ if ($mode{static}){
if ($mode{cflags} || $mode{libs} || $mode{variable}) {
push @vlist, do_cflags($dep_cfg_list) if $mode{cflags};
push @vlist, do_libs($dep_cfg_list) if $mode{libs};
- print join(' ', @vlist), "\n" if $rc == 0;
+ say join(' ', @vlist) if $rc == 0;
}
exit $rc;
###
-sub handle_config
+sub handle_config($p, $op, $v, $list)
{
- my ($p, $op, $v, $list) = @_;
my $cfg = cache_find_config($p);
unshift @$list, $p if defined $cfg;
@@ -316,7 +321,7 @@ sub handle_config
my $deps = $cfg->get_property($property, $variables);
return unless defined $deps;
for my $dep (@$deps) {
- if ($dep =~
m/^(.*?)\s*([<=>]+)\s*([\d\.]+|[\d\.]+[\w]*[\d]+)$/) {
+ if ($dep =~
m/^(.*?)\s*([<=>]+)\s*([\d\.]+|[\d\.]+\w*\d+)$/) {
handle_config($1, $2, $3, $list);
} else {
handle_config($dep, undef, undef, $list);
@@ -339,10 +344,8 @@ sub handle_config
# look for the .pc file in each of the PKGPATH elements.
Re: iked: ibuf saga step 2
On Fri, May 26, 2023 at 10:11:32AM +0200, Claudio Jeker wrote:
> Kill ibuf_prepend() it is used only once and the function does unholy
> things to the ibuf passed in. Just do the obivous dance in the callee.
> The only thing to be careful about is the fact that all pointers of buf
> are replaced (msg->msg_data).
>
> Tested with iked -t (which should use this codepath).
What can I say. Unholy is a nice way of putting it. This is rather
horrible. I agree, inlining it makes sense.
It is a bit scary to do ibuf_free(buf); (which ibuf_prepend() is careful
not to do) since that is passed in by the caller. In turn, pointers are
no longer transferred via memcpy, which is a good thing in my book...
Arguably, ownership of the caller's buf is transferred to resp.msg_data
in all cases, so that is fine except that it is still a bit of a trap.
If this was my code I would probably insist on invalidating the buf in
the caller after hanging it onto resp.
ok tb
But please fix the leak indicated below.
> --
> :wq Claudio
>
> Index: iked.h
> ===
> RCS file: /cvs/src/sbin/iked/iked.h,v
> retrieving revision 1.213
> diff -u -p -r1.213 iked.h
> --- iked.h23 May 2023 13:57:14 - 1.213
> +++ iked.h26 May 2023 07:21:43 -
> @@ -1279,7 +1279,6 @@ struct ibuf *
>ibuf_dup(struct ibuf *);
> struct ibuf *
>ibuf_random(size_t);
> -int ibuf_prepend(struct ibuf *, void *, size_t);
> int ibuf_strcat(struct ibuf **, const char *);
> int ibuf_strlen(struct ibuf *);
>
> Index: ikev2_msg.c
> ===
> RCS file: /cvs/src/sbin/iked/ikev2_msg.c,v
> retrieving revision 1.92
> diff -u -p -r1.92 ikev2_msg.c
> --- ikev2_msg.c 23 May 2023 13:57:14 - 1.92
> +++ ikev2_msg.c 26 May 2023 07:20:13 -
> @@ -290,10 +290,17 @@ ikev2_msg_send(struct iked *env, struct
> ibuf_length(buf), isnatt ? ", NAT-T" : "");
>
> if (isnatt) {
> - if (ibuf_prepend(buf, , sizeof(natt)) == -1) {
> + struct ibuf *new;
> + if ((new = ibuf_new(, sizeof(natt))) == NULL) {
> log_debug("%s: failed to set NAT-T", __func__);
> return (-1);
> }
> + if (ibuf_cat(new, buf) == -1) {
This needs an
ibuf_free(new);
> + log_debug("%s: failed to set NAT-T", __func__);
> + return (-1);
> + }
> + ibuf_free(buf);
> + buf = msg->msg_data = new;
> }
>
> if (sendtofrom(msg->msg_fd, ibuf_data(buf), ibuf_size(buf), 0,
> Index: imsg_util.c
> ===
> RCS file: /cvs/src/sbin/iked/imsg_util.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 imsg_util.c
> --- imsg_util.c 23 May 2023 13:57:14 - 1.16
> +++ imsg_util.c 26 May 2023 07:21:26 -
> @@ -146,25 +146,6 @@ ibuf_setsize(struct ibuf *buf, size_t le
> }
>
> int
> -ibuf_prepend(struct ibuf *buf, void *data, size_t len)
> -{
> - struct ibuf *new;
> -
> - /* Swap buffers (we could also use memmove here) */
> - if ((new = ibuf_new(data, len)) == NULL)
> - return (-1);
> - if (ibuf_cat(new, buf) == -1) {
> - ibuf_free(new);
> - return (-1);
> - }
> - free(buf->buf);
> - memcpy(buf, new, sizeof(*buf));
> - free(new);
> -
> - return (0);
> -}
> -
> -int
> ibuf_strcat(struct ibuf **buf, const char *s)
> {
> size_t slen;
>
iked: ibuf saga step 2
Kill ibuf_prepend() it is used only once and the function does unholy
things to the ibuf passed in. Just do the obivous dance in the callee.
The only thing to be careful about is the fact that all pointers of buf
are replaced (msg->msg_data).
Tested with iked -t (which should use this codepath).
--
:wq Claudio
Index: iked.h
===
RCS file: /cvs/src/sbin/iked/iked.h,v
retrieving revision 1.213
diff -u -p -r1.213 iked.h
--- iked.h 23 May 2023 13:57:14 - 1.213
+++ iked.h 26 May 2023 07:21:43 -
@@ -1279,7 +1279,6 @@ struct ibuf *
ibuf_dup(struct ibuf *);
struct ibuf *
ibuf_random(size_t);
-int ibuf_prepend(struct ibuf *, void *, size_t);
int ibuf_strcat(struct ibuf **, const char *);
int ibuf_strlen(struct ibuf *);
Index: ikev2_msg.c
===
RCS file: /cvs/src/sbin/iked/ikev2_msg.c,v
retrieving revision 1.92
diff -u -p -r1.92 ikev2_msg.c
--- ikev2_msg.c 23 May 2023 13:57:14 - 1.92
+++ ikev2_msg.c 26 May 2023 07:20:13 -
@@ -290,10 +290,17 @@ ikev2_msg_send(struct iked *env, struct
ibuf_length(buf), isnatt ? ", NAT-T" : "");
if (isnatt) {
- if (ibuf_prepend(buf, , sizeof(natt)) == -1) {
+ struct ibuf *new;
+ if ((new = ibuf_new(, sizeof(natt))) == NULL) {
log_debug("%s: failed to set NAT-T", __func__);
return (-1);
}
+ if (ibuf_cat(new, buf) == -1) {
+ log_debug("%s: failed to set NAT-T", __func__);
+ return (-1);
+ }
+ ibuf_free(buf);
+ buf = msg->msg_data = new;
}
if (sendtofrom(msg->msg_fd, ibuf_data(buf), ibuf_size(buf), 0,
Index: imsg_util.c
===
RCS file: /cvs/src/sbin/iked/imsg_util.c,v
retrieving revision 1.16
diff -u -p -r1.16 imsg_util.c
--- imsg_util.c 23 May 2023 13:57:14 - 1.16
+++ imsg_util.c 26 May 2023 07:21:26 -
@@ -146,25 +146,6 @@ ibuf_setsize(struct ibuf *buf, size_t le
}
int
-ibuf_prepend(struct ibuf *buf, void *data, size_t len)
-{
- struct ibuf *new;
-
- /* Swap buffers (we could also use memmove here) */
- if ((new = ibuf_new(data, len)) == NULL)
- return (-1);
- if (ibuf_cat(new, buf) == -1) {
- ibuf_free(new);
- return (-1);
- }
- free(buf->buf);
- memcpy(buf, new, sizeof(*buf));
- free(new);
-
- return (0);
-}
-
-int
ibuf_strcat(struct ibuf **buf, const char *s)
{
size_t slen;
