pair: send without kernel lock
pair(4)'s output path can run without kernel lock just fine.
NB: Looking at CVS log, it seems this was not done during import because
IFXF_MPSSAFE became a thing afterwards.
Feedback? Objections? OK?
Index: if_pair.c
===
RCS file: /cvs/src/sys/net/if_pair.c,v
retrieving revision 1.16
diff -u -p -r1.16 if_pair.c
--- if_pair.c 21 Aug 2020 22:59:27 - 1.16
+++ if_pair.c 7 Jan 2021 00:20:20 -
@@ -37,7 +37,7 @@
void pairattach(int);
intpairioctl(struct ifnet *, u_long, caddr_t);
-void pairstart(struct ifnet *);
+void pairstart(struct ifqueue *);
intpair_clone_create(struct if_clone *, int);
intpair_clone_destroy(struct ifnet *);
intpair_media_change(struct ifnet *);
@@ -116,8 +116,8 @@ pair_clone_create(struct if_clone *ifc,
ifp->if_softc = sc;
ifp->if_ioctl = pairioctl;
- ifp->if_start = pairstart;
- ifp->if_xflags = IFXF_CLONED;
+ ifp->if_qstart = pairstart;
+ ifp->if_xflags = IFXF_CLONED | IFXF_MPSAFE;
ifp->if_hardmtu = ETHER_MAX_HARDMTU_LEN;
ifp->if_capabilities = IFCAP_VLAN_MTU;
@@ -158,8 +158,9 @@ pair_clone_destroy(struct ifnet *ifp)
}
void
-pairstart(struct ifnet *ifp)
+pairstart(struct ifqueue *ifq)
{
+ struct ifnet*ifp = ifq->ifq_if;
struct pair_softc *sc = (struct pair_softc *)ifp->if_softc;
struct mbuf_list ml = MBUF_LIST_INITIALIZER();
struct ifnet*pairedifp;
@@ -167,11 +168,7 @@ pairstart(struct ifnet *ifp)
pairedifp = if_get(sc->sc_pairedif);
- for (;;) {
- m = ifq_dequeue(>if_snd);
- if (m == NULL)
- break;
-
+ while ((m = ifq_dequeue(ifq)) != NULL) {
#if NBPFILTER > 0
if (ifp->if_bpf)
bpf_mtap(ifp->if_bpf, m, BPF_DIRECTION_OUT);
Re: tpm(4): don't use tvtohz(9)
> Date: Wed, 6 Jan 2021 16:16:27 -0600
> From: Scott Cheloha
>
> On Wed, Jan 06, 2021 at 12:16:13PM -0600, Scott Cheloha wrote:
> > As mentioned in a prior mail, tpm(4) is the last user of tvtohz(9) in
> > the tree.
> >
> > However, we don't need to use tvtohz(9) in tpm(4) at all. Converting
> > from milliseconds to ticks is trivial. Using an intermediary timeval
> > is just pointless indirection.
> >
> > With this committed I will be able to remove both tvtohz(9) and
> > tstohz(9) from the tree in a subsequent patch.
>
> Whoops, made a mistake in the prior diff.
>
> We want to MAX() the result up to 1 to avoid dividing to zero and
> blocking indefinitely in e.g. tsleep(9). Previous diff MIN'd the
> result down to 1, which is very wrong.
To be honest I'd just zap the function completely and instead simply do:
to = TPM_ACCESS_TMO / 10;
and
to = TPM_BURST_TMO / 10;
There is no magic happening here. The code is just doing a hardware
register poll loop in 10us steps.
Hmm, actually it seems the code is broken and uses steps of 10
microseconds instead of milliseconds. So instead it should probably
use:
to = TPM_ACCESS_TMO * 100;
and
to = TPM_BURST_TMO * 100;
Cheers,
Mark
> Index: tpm.c
> ===
> RCS file: /cvs/src/sys/dev/acpi/tpm.c,v
> retrieving revision 1.10
> diff -u -p -r1.10 tpm.c
> --- tpm.c 22 May 2020 10:16:37 - 1.10
> +++ tpm.c 6 Jan 2021 22:09:49 -
> @@ -24,6 +24,7 @@
> */
>
> #include
> +#include
> #include
> #include
> #include
> @@ -455,12 +456,7 @@ tpm_status(struct tpm_softc *sc)
> int
> tpm_tmotohz(int tmo)
> {
> - struct timeval tv;
> -
> - tv.tv_sec = tmo / 1000;
> - tv.tv_usec = 1000 * (tmo % 1000);
> -
> - return tvtohz();
> + return MAX(1, tmo * hz / 1000);
> }
>
> int
>
>
Re: tpm(4): don't use tvtohz(9)
On Wed, Jan 06, 2021 at 12:16:13PM -0600, Scott Cheloha wrote:
> As mentioned in a prior mail, tpm(4) is the last user of tvtohz(9) in
> the tree.
>
> However, we don't need to use tvtohz(9) in tpm(4) at all. Converting
> from milliseconds to ticks is trivial. Using an intermediary timeval
> is just pointless indirection.
>
> With this committed I will be able to remove both tvtohz(9) and
> tstohz(9) from the tree in a subsequent patch.
Whoops, made a mistake in the prior diff.
We want to MAX() the result up to 1 to avoid dividing to zero and
blocking indefinitely in e.g. tsleep(9). Previous diff MIN'd the
result down to 1, which is very wrong.
Index: tpm.c
===
RCS file: /cvs/src/sys/dev/acpi/tpm.c,v
retrieving revision 1.10
diff -u -p -r1.10 tpm.c
--- tpm.c 22 May 2020 10:16:37 - 1.10
+++ tpm.c 6 Jan 2021 22:09:49 -
@@ -24,6 +24,7 @@
*/
#include
+#include
#include
#include
#include
@@ -455,12 +456,7 @@ tpm_status(struct tpm_softc *sc)
int
tpm_tmotohz(int tmo)
{
- struct timeval tv;
-
- tv.tv_sec = tmo / 1000;
- tv.tv_usec = 1000 * (tmo % 1000);
-
- return tvtohz();
+ return MAX(1, tmo * hz / 1000);
}
int
Re: New ujoy(4) device for USB gamecontrollers
On Mon, Dec 28, 2020 at 05:03:14PM -0700, Thomas Frohwein wrote: > Hi, > > This is a diff to propose a new device type for USB gamecontrollers, > 'ujoy'. > > Rationale > - > > Since the tightening of security around USB devices, USB > gamecontrollers that generally attach to the kitchen-sink uhid device > don't work out of the box anymore since read permissions are absent. As > a consequence, USB gamecontrollers don't work out of the box anymore; > requiring a root user to find what uhid device the controller is > attached to and chmod +r that one. This poses a barrier to using those > devices, as well as in my opinion a security risk by enticing users to > indiscriminately do '# chmod +r /dev/uhid*' in order to not have to > re-apply permissions on different uhid devices multiple times. > > The proposed solution is inspired by the implementation of fido(4) [1]; > a similarly narrow use case for a certain type of USB devices. It > creates a device for gamecontrollers that has the minimum required > permissions, but won't require chmod(1)'ing device files to get the > controller working. > > Implementation in this Diff > --- > > This diff is largely based on reyk's commit for the fido(4) device from > December 2019 [1]. It creates a ujoy device and attaches USB devices > identified as HUG_JOYSTICK or HUG_GAME_PAD. > > ujoyopen() is set up such that it only allows read-only access. If it is > opened with write permissions, it should error with EPERM. Only a > subset of ioctls is allowed in ujoyioctl(), based on what I could find > that is used in devel/sdl2 (USB_GET_DEVICEINFO,USB_GET_REPORT, > USB_GET_REPORT_DESC,USB_GET_REPORT_ID), as well as FIONBIO and > FIOASYNC. > > This contains conf.c and MAKEDEV.md bits for the same architectures as > fido(4). A small man page, as well as updates to other pertinent man > pages (usb(4), uhidev(4)) are included, again following the example of > the commits for the fido(4) device. > > Credit to brynet@ who helped me extensively with this diff. > > Testing > --- > > A simple way to test this without needing to do a full release is > building a kernel with the diff and booting into it, then creating the > updated MAKEDEV script with: > > $ cd /usr/src/etc/etc.amd64/ > $ make# this will create MAKEDEV in etc.amd64 > $ cd /dev > # sh /usr/src/etc/etc.amd64/MAKEDEV ujoy > > This creates 2 device ujoy/0 and ujoy/1 by default. The device is > read-only by default (444). > > Plug in your USB gamecontroller and check in dmesg that it attaches as > ujoy, not uhid. > > The simplest way to test is with usbhidctl(1): > > $ usbhidctl -f /dev/ujoy/0 -l > > This will loop through printing the state of buttons, sticks etc. until > exited with Crtl-C. > > Another way to test is with games/sdl-jstest. As the SDL backends use > /dev/uhid* to find gamecontrollers, the way I tested this is by > repurposing /dev/uhid0 with mknod(8) (note major 100 is for amd64): > > $ cd /dev > # rm uhid0 > # mknod -m 444 uhid0 c 100 0 > > I put the diff through a release(8) without xenocara on amd64, and it > built base without problems and /dev/MAKEDEV and the /dev/ujoy directory > all look correct. > > I have tested usbhidctl and sdl-jstest with an XBox 360 gamepad and the > Logitech F310 gamepad, the latter both in DInput and XInput mode. All > of those work as expected. > > Issues/Follow-up Tasks > -- > > As ujoy devices don't attach to /dev/uhid* anymore, ports that use > gamecontrollers will need to be patched. This seems to be mostly (if > not exclusively) sdl/sdl2-based ports, so I anticipate that patching > these 2 ports will take care of the bulk of the use cases. > > On other OS's, writing to gamecontrollers is used at times for the > "rumble" functionality of the controller, or setting LEDs (like the > circle LED on XBox 360 controllers that can reflect what player number > the gamepad is for). Those have to my knowledge never worked on OpenBSD > and are not of any immediate interest as far as I can tell. > > ok? comments? The implementation as such looks fine to me. But I quickly gave the diff a spin before on amd64 using my PS controller, and the result is not what I would expect. With uhid, I can access the controller on /dev/uhid6. The attach looks like this: imac /bsd: uaudio0 at uhub4 port 4 configuration 1 interface 1 "Sony Interactive Entertainment Wireless Controller" rev 2.00/1.00 addr 6 imac /bsd: uaudio0: class v1, full-speed, sync, channels: 2 play, 1 rec, 4 ctls imac /bsd: audio1 at uaudio0 imac /bsd: uhidev6 at uhub4 port 4 configuration 1 interface 3 "Sony Interactive Entertainment Wireless Controller" rev 2.00/1.00 addr 6 imac /bsd: uhidev6: iclass 3/0, 180 report ids imac /bsd: uhid6 at uhidev6 reportid 1: input=63, output=0, feature=0 imac /bsd: uhid7 at uhidev6 reportid 2: input=0, output=0, feature=36 imac /bsd: uhid8 at uhidev6 reportid 4: input=0,
Re: fix opsfd parse.y shit/reduce conflicts
Thanks,
i think the dependon might have been my fault.
code reads ok.
I also checked a few configs, including an artificial one that uses depend
on.
/Benno
Claudio Jeker(cje...@diehard.n-r-g.com) on 2021.01.06 11:11:49 +0100:
> The dependon statement in ospfd parse.y introduces some troubles since it
> holds an empty rule that then conflicts with optnl.
> This diff changes dependon into dependon and dependonopt so that in the
> place where it is optional dependonopt can be used and in the places where
> it must not be optional it isn't. With this the shift/reduce conficts are
> gone. While at it cleanup some other rules and use the same optnl idiom
> for area and interface (it is the same one as used by bgpd).
>
> Please test this with your configs to see if this causes any parse errors
> (ospfd -n should be enough for this).
> --
> :wq Claudio
>
>
> Index: parse.y
> ===
> RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v
> retrieving revision 1.101
> diff -u -p -r1.101 parse.y
> --- parse.y 29 Dec 2020 19:44:47 - 1.101
> +++ parse.y 6 Jan 2021 10:10:23 -
> @@ -144,7 +144,7 @@ typedef struct {
> %token NUMBER
> %type yesno no optlist optlist_l option demotecount
> msec
> %type deadtime
> -%type string dependon
> +%type string dependon dependonopt
> %type redistribute
> %type areaid
>
> @@ -297,7 +297,7 @@ conf_main : ROUTERID STRING {
> ;
>
>
> -redistribute : no REDISTRIBUTE NUMBER '/' NUMBER optlist dependon {
> +redistribute : no REDISTRIBUTE NUMBER '/' NUMBER optlist dependonopt {
> struct redistribute *r;
>
> if ((r = calloc(1, sizeof(*r))) == NULL)
> @@ -323,7 +323,7 @@ redistribute : no REDISTRIBUTE NUMBER '/
> free($7);
> $$ = r;
> }
> - | no REDISTRIBUTE STRING optlist dependon {
> + | no REDISTRIBUTE STRING optlist dependonopt {
> struct redistribute *r;
>
> if ((r = calloc(1, sizeof(*r))) == NULL)
> @@ -426,8 +426,10 @@ option : METRIC NUMBER {
> }
> ;
>
> -dependon : /* empty */ { $$ = NULL; }
> - | DEPEND ON STRING {
> +dependonopt : /* empty */ { $$ = NULL; }
> + | dependon
> +
> +dependon : DEPEND ON STRING {
> struct in_addr addr;
> struct kif *kif;
>
> @@ -599,7 +601,7 @@ area : AREA areaid {
> memcpy(, defs, sizeof(areadefs));
> md_list_copy(_list, >md_list);
> defs =
> - } '{' optnl areaopts_l '}' {
> + } '{' optnl areaopts_l optnl '}' {
> area = NULL;
> md_list_clr(>md_list);
> defs =
> @@ -627,8 +629,8 @@ areaid: NUMBER {
> }
> ;
>
> -areaopts_l : areaopts_l areaoptsl nl
> - | areaoptsl optnl
> +areaopts_l : areaopts_l nl areaoptsl
> + | areaoptsl
> ;
>
> areaoptsl: interface
> @@ -739,13 +741,13 @@ interface : INTERFACE STRING {
> }
> ;
>
> -interface_block : '{' optnl interfaceopts_l '}'
> +interface_block : '{' optnl interfaceopts_l optnl '}'
> | '{' optnl '}'
> - |
> + | /* empty */
> ;
>
> -interfaceopts_l : interfaceopts_l interfaceoptsl nl
> - | interfaceoptsl optnl
> +interfaceopts_l : interfaceopts_l nl interfaceoptsl
> + | interfaceoptsl
> ;
>
> interfaceoptsl : PASSIVE { iface->passive = 1; }
>
Re: video(4) multiple opens
On Wed, Jan 06 2021, Marcus Glocker wrote: > On Tue, Jan 05, 2021 at 11:54:31PM +0100, Jeremie Courreges-Anglas wrote: [...] >> Here's the diff. IIUC the use of atomic operations isn't strictly >> needed here since open(2) runs with the kernel lock, but the result >> is easy to understand IMO. > > I don't agree with that, see my comments bellow in point a) and b). > >> Thoughts? ok? > > I think it doesn't harm to allow the same process to do multiple opens > on video(1) as a first improvement. But I'm not happy using > atomic_cas_ptr(9) and atomic_swap_ptr(9) for this because: > > a) As you already have mentioned, we don't require atomic > operations here. I checked that those functions are very > rarely used in the kernel, and only there were atomic > operation is required. I'm afraid that when we use those in > video(1), and somebody looks at the code later on to > eventually replace it, there will be confusion if there was > a specific reason why to use atomic functions. > > b) IMO it doesn't simplify the code. I first had to read the > manual pages to understand how those functions work. Fair enough, I also felt that it was a bit premature. > I would prefer to do something simpler, like in this adapted diff. > If it works for you, I'm OK if you commit this instead. Done, thanks. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: tpm(4): don't use tvtohz(9)
On Wed, Jan 06, 2021 at 12:16:13PM -0600, Scott Cheloha wrote: > As mentioned in a prior mail, tpm(4) is the last user of tvtohz(9) in > the tree. > > However, we don't need to use tvtohz(9) in tpm(4) at all. Converting > from milliseconds to ticks is trivial. Using an intermediary timeval > is just pointless indirection. > > With this committed I will be able to remove both tvtohz(9) and > tstohz(9) from the tree in a subsequent patch. OK kn
tpm(4): don't use tvtohz(9)
As mentioned in a prior mail, tpm(4) is the last user of tvtohz(9) in
the tree.
However, we don't need to use tvtohz(9) in tpm(4) at all. Converting
from milliseconds to ticks is trivial. Using an intermediary timeval
is just pointless indirection.
With this committed I will be able to remove both tvtohz(9) and
tstohz(9) from the tree in a subsequent patch.
ok?
Index: tpm.c
===
RCS file: /cvs/src/sys/dev/acpi/tpm.c,v
retrieving revision 1.10
diff -u -p -r1.10 tpm.c
--- tpm.c 22 May 2020 10:16:37 - 1.10
+++ tpm.c 6 Jan 2021 18:06:47 -
@@ -24,6 +24,7 @@
*/
#include
+#include
#include
#include
#include
@@ -455,12 +456,7 @@ tpm_status(struct tpm_softc *sc)
int
tpm_tmotohz(int tmo)
{
- struct timeval tv;
-
- tv.tv_sec = tmo / 1000;
- tv.tv_usec = 1000 * (tmo % 1000);
-
- return tvtohz();
+ return MIN(1, tmo * hz / 1000);
}
int
Re: sleep(3): don't bypass nanosleep(2)
On Wed, 06 Jan 2021 10:31:28 -0600, Scott Cheloha wrote: > Other benefits: > > - sleep(3) now *always* shows up in ktrace. > > - sleep(3) with a zero input now blocks for up to 1 tick, just like > nanosleep(2) does with a zero input (more intuitive behavior). > > - Neither NetBSD nor FreeBSD bypass nanosleep(2) like this, so now our > sleep(3) is more like theirs. OK millert@ - todd
sleep(3): don't bypass nanosleep(2)
In sleep(3), if seconds is zero we don't call nanosleep(2).
I don't like this. If sleep(3) really is a simplified interface to
nanosleep(2) (as we claim in the manpage) we should let nanosleep(2)
handle the input and make decisions.
Other benefits:
- sleep(3) now *always* shows up in ktrace.
- sleep(3) with a zero input now blocks for up to 1 tick, just like
nanosleep(2) does with a zero input (more intuitive behavior).
- Neither NetBSD nor FreeBSD bypass nanosleep(2) like this, so now our
sleep(3) is more like theirs.
ok?
Index: sleep.c
===
RCS file: /cvs/src/lib/libc/gen/sleep.c,v
retrieving revision 1.12
diff -u -p -r1.12 sleep.c
--- sleep.c 14 Dec 2009 05:10:13 - 1.12
+++ sleep.c 6 Jan 2021 16:21:11 -
@@ -36,9 +36,6 @@ sleep(unsigned int seconds)
{
struct timespec rqt, rmt;
- if (seconds == 0)
- return(0);
-
rqt.tv_sec = seconds;
rqt.tv_nsec = 0;
Re: fix opsfd parse.y shit/reduce conflicts
On 06/01/2021 12:11, Claudio Jeker wrote: The dependon statement in ospfd parse.y introduces some troubles since it holds an empty rule that then conflicts with optnl. This diff changes dependon into dependon and dependonopt so that in the place where it is optional dependonopt can be used and in the places where it must not be optional it isn't. With this the shift/reduce conficts are gone. While at it cleanup some other rules and use the same optnl idiom for area and interface (it is the same one as used by bgpd). Please test this with your configs to see if this causes any parse errors (ospfd -n should be enough for this). ./ospfd -n configuration OK I have depend on carpXXX, on 2 interfaces G
Re: Make ospf6d work on point-to-point links
On 06/01/2021 14:02, Claudio Jeker wrote: The code in ospf6d is a bit broken when it comes to point-to-point links. This diff fixes this by a) using the neighbor address instead of the unset interface destination address and by b) matching the incomming packet against all possible IPs of that interface. I tripped on b) because my P2P interface has more than one link-local address and the code just likes to select the wrong one. This works for my case, please check I did not break something else. With this, the annoying send_ls_update: Network is unreachable send_packet: error sending packet on interface vlanXXX to ::: Network is unreachable is gone :) other than that, I didn't have any other problem with p2p, neither have now. Failover worked fine, demote worked fine. G
Re: video(4) multiple opens
On Tue, Jan 05, 2021 at 11:54:31PM +0100, Jeremie Courreges-Anglas wrote:
>
> I hit a weird failure with firefox and BigBlueButton
> (https://bigbluebutton.org/) where firefox can't use my webcam.
> video(1) works, same for other webrtc sites in firefox, eg meet.jit.si.
> ktrace shows that a single firefox process tries to open /dev/video0
> more than once, and that fails with EBUSY. The code lies in the
> libwebrtc library
>
>
> https://webrtc.googlesource.com/src/+/refs/heads/master/modules/video_capture/linux/
>
> In my tests the multiple open() calls only happen at initialization
> time, video streaming only uses one fd.
>
> Back to our kernel, the current restrictive behavior was introduced with
>
> revision 1.18
> date: 2008/07/23 22:10:21; author: mglocker; state: Exp; lines: +11 -4;
> If /dev/video* is already used by an application, return EBUSY to other
> applications. Fixes a kernel panic.
>
> Reported by ian@
>
> Meanwhile, the V4L2 API specifies that a device "can be opened more than
> once" from multiple processes, with access to certain methods blocked
> when an application starts reading from the device.
>
>
> https://www.kernel.org/doc/html/v5.10/userspace-api/media/v4l/open.html#multiple-opens
>
> So the API says we're being too restrictive, but we don't want to go
> back to uncontrolled access either. I guess the ideal solution would be
> to implement multiple process access with fine-grained checks, but...
> I don't have time for that!
Oh, I guess that makes video(1) an "old and obscure" driver according
to the API [2] footnote ;-)
Finally we should implement the controls for multiple video device
openings based on what has been exactly requested. Since the basic
idea mentioned in the API is that other processes, like a panel
application, can be started to e.g. change controls. I will have a
look at that when I find some time.
> An easy improvement for my use case would be to allow the same process
> to open a device multiple times. It fixes firefox + BigBlueButton for
> me and doesn't make concurrent accesses worse (multiple threads from the
> same process can already do concurrent accesses, which is something
> we might want to look at).
Interesting. I was not aware that threads of the same process can do
that. I need to have a closer look at what is happening here.
> Here's the diff. IIUC the use of atomic operations isn't strictly
> needed here since open(2) runs with the kernel lock, but the result
> is easy to understand IMO.
I don't agree with that, see my comments bellow in point a) and b).
> Thoughts? ok?
I think it doesn't harm to allow the same process to do multiple opens
on video(1) as a first improvement. But I'm not happy using
atomic_cas_ptr(9) and atomic_swap_ptr(9) for this because:
a) As you already have mentioned, we don't require atomic
operations here. I checked that those functions are very
rarely used in the kernel, and only there were atomic
operation is required. I'm afraid that when we use those in
video(1), and somebody looks at the code later on to
eventually replace it, there will be confusion if there was
a specific reason why to use atomic functions.
b) IMO it doesn't simplify the code. I first had to read the
manual pages to understand how those functions work.
I would prefer to do something simpler, like in this adapted diff.
If it works for you, I'm OK if you commit this instead.
> Index: dev/video.c
> ===
> RCS file: /d/cvs/src/sys/dev/video.c,v
> retrieving revision 1.46
> diff -u -p -r1.46 video.c
> --- dev/video.c 28 Dec 2020 18:28:11 - 1.46
> +++ dev/video.c 5 Jan 2021 22:34:04 -
> @@ -28,6 +28,8 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
>
> #include
> @@ -46,8 +48,7 @@ struct video_softc {
> struct device *sc_dev;/* hardware device struct */
> struct video_hw_if *hw_if; /* hardware interface */
> char sc_dying; /* device detached */
> -#define VIDEO_OPEN 0x01
> - char sc_open;
> + struct process *sc_owner; /* owner process */
>
> int sc_fsize;
> uint8_t *sc_fbuffer;
> @@ -101,6 +102,7 @@ videoattach(struct device *parent, struc
> sc->hw_hdl = sa->hdl;
> sc->sc_dev = parent;
> sc->sc_fbufferlen = 0;
> + sc->sc_owner = NULL;
>
> if (sc->hw_if->get_bufsize)
> sc->sc_fbufferlen = (sc->hw_if->get_bufsize)(sc->hw_hdl);
> @@ -121,6 +123,7 @@ videoopen(dev_t dev, int flags, int fmt,
> {
> int unit;
> struct video_softc *sc;
> + struct process *owner;
>
> unit = VIDEOUNIT(dev);
> if (unit >= video_cd.cd_ndevs ||
> @@ -128,9 +131,13 @@
Re: ntpd.conf.5 duplicate word
On Wed, Jan 06, 2021 at 08:53:04PM +0800, Sean Davies wrote: > Index: ntpd.conf.5 > === > RCS file: /cvs/src/usr.sbin/ntpd/ntpd.conf.5,v > retrieving revision 1.46 > diff -u -p -u -r1.46 ntpd.conf.5 > --- ntpd.conf.5 16 May 2020 16:58:12 - 1.46 > +++ ntpd.conf.5 6 Jan 2021 12:41:49 - > @@ -119,7 +119,7 @@ sensor udcf0 correction 7 > A > .Ic refid > .Ar ID-string > -of up up to 4 ASCII characters can be > +of up to 4 ASCII characters can be > given to publish the sensor type to clients. > RFC 2030 suggests some common reference identifiers, but new identifiers > "can be contrived as appropriate." > > -- > Sean > fixed, thanks. jmc
ntpd.conf.5 duplicate word
Index: ntpd.conf.5 === RCS file: /cvs/src/usr.sbin/ntpd/ntpd.conf.5,v retrieving revision 1.46 diff -u -p -u -r1.46 ntpd.conf.5 --- ntpd.conf.5 16 May 2020 16:58:12 - 1.46 +++ ntpd.conf.5 6 Jan 2021 12:41:49 - @@ -119,7 +119,7 @@ sensor udcf0 correction 7 A .Ic refid .Ar ID-string -of up up to 4 ASCII characters can be +of up to 4 ASCII characters can be given to publish the sensor type to clients. RFC 2030 suggests some common reference identifiers, but new identifiers "can be contrived as appropriate." -- Sean
Re: unbound: missing null check
On 2021/01/06 10:24, Florian Obser wrote:
> On Wed, Jan 06, 2021 at 10:11:01AM +0100, Anton Lindqvist wrote:
> > Hi,
> > I have a unbound forward zone configured on my router for my $DAYJOB.
> > The address associated with the zone is only accessible when the router
> > is connected to a VPN. If the VPN connection is absent, trying to
> > resolve any domain that must be handled by the zone crashes unbound.
> > Turns out there's a missing NULL check in comm_point_send_udp_msg().
> > The same routine already has `if (addr) {} else {}' branches so I guess
> > protecting the call to log_addr() using the same conditional is
> > reasonable.
> >
> > Should this instead be upstreamed? Comments? OK?
>
> yes, please upstream it.
>
> Could you also do sbin/unwind/libunbound/util/netevent.c?
>
> OK florian
Already committed but I'll add my OK too anyway ;) Thanks for tracking
this down.
>
> >
> > Index: util/netevent.c
> > ===
> > RCS file: /cvs/src/usr.sbin/unbound/util/netevent.c,v
> > retrieving revision 1.27
> > diff -u -p -r1.27 netevent.c
> > --- util/netevent.c 10 Dec 2020 21:44:58 - 1.27
> > +++ util/netevent.c 6 Jan 2021 09:03:59 -
> > @@ -379,8 +379,9 @@ comm_point_send_udp_msg(struct comm_poin
> > if(!udp_send_errno_needs_log(addr, addrlen))
> > return 0;
> > verbose(VERB_OPS, "sendto failed: %s", sock_strerror(errno));
> > - log_addr(VERB_OPS, "remote address is",
> > - (struct sockaddr_storage*)addr, addrlen);
> > + if(addr)
> > + log_addr(VERB_OPS, "remote address is",
> > + (struct sockaddr_storage*)addr, addrlen);
> > return 0;
> > } else if((size_t)sent != sldns_buffer_remaining(packet)) {
> > log_err("sent %d in place of %d bytes",
> >
>
> --
> I'm not entirely sure you are real.
>
Make ospf6d work on point-to-point links
The code in ospf6d is a bit broken when it comes to point-to-point links.
This diff fixes this by a) using the neighbor address instead of the unset
interface destination address and by b) matching the incomming packet
against all possible IPs of that interface.
I tripped on b) because my P2P interface has more than one link-local
address and the code just likes to select the wrong one.
This works for my case, please check I did not break something else.
--
:wq Claudio
Index: lsupdate.c
===
RCS file: /cvs/src/usr.sbin/ospf6d/lsupdate.c,v
retrieving revision 1.18
diff -u -p -r1.18 lsupdate.c
--- lsupdate.c 15 Jul 2020 14:47:41 - 1.18
+++ lsupdate.c 6 Jan 2021 11:28:43 -
@@ -474,7 +474,7 @@ ls_retrans_timer(int fd, short event, vo
/* ls_retrans_list_free retriggers the timer */
return;
} else if (nbr->iface->type == IF_TYPE_POINTOPOINT)
- memcpy(, >iface->dst, sizeof(addr));
+ memcpy(, >addr, sizeof(addr));
else
inet_pton(AF_INET6, AllDRouters, );
} else
Index: packet.c
===
RCS file: /cvs/src/usr.sbin/ospf6d/packet.c,v
retrieving revision 1.17
diff -u -p -r1.17 packet.c
--- packet.c23 Dec 2019 07:33:49 - 1.17
+++ packet.c6 Jan 2021 11:52:08 -
@@ -82,12 +82,9 @@ send_packet(struct iface *iface, struct
struct in6_addr *dst)
{
struct sockaddr_in6 sa6;
- struct msghdr msg;
- struct ioveciov[1];
- /* setup buffer */
+ /* setup sockaddr */
bzero(, sizeof(sa6));
-
sa6.sin6_family = AF_INET6;
sa6.sin6_len = sizeof(sa6);
sa6.sin6_addr = *dst;
@@ -104,15 +101,8 @@ send_packet(struct iface *iface, struct
return (-1);
}
- bzero(, sizeof(msg));
- msg.msg_name =
- msg.msg_namelen = sizeof(sa6);
- iov[0].iov_base = buf->buf;
- iov[0].iov_len = ibuf_size(buf);
- msg.msg_iov = iov;
- msg.msg_iovlen = 1;
-
- if (sendmsg(iface->fd, , 0) == -1) {
+ if (sendto(iface->fd, buf->buf, ibuf_size(buf), 0,
+ (struct sockaddr *), sizeof(sa6)) == -1) {
log_warn("send_packet: error sending packet on interface %s",
iface->name);
return (-1);
@@ -186,11 +176,16 @@ recv_packet(int fd, short event, void *b
* AllDRouters is only valid for DR and BDR but this is checked later.
*/
inet_pton(AF_INET6, AllSPFRouters, );
-
if (!IN6_ARE_ADDR_EQUAL(, )) {
inet_pton(AF_INET6, AllDRouters, );
if (!IN6_ARE_ADDR_EQUAL(, )) {
- if (!IN6_ARE_ADDR_EQUAL(, >addr)) {
+ struct iface_addr *ia;
+
+ TAILQ_FOREACH(ia, >ifa_list, entry) {
+ if (IN6_ARE_ADDR_EQUAL(, >addr))
+ break;
+ }
+ if (ia == NULL) {
log_debug("recv_packet: packet sent to wrong "
"address %s, interface %s",
log_in6addr(), iface->name);
Re: unbound: missing null check
On Wed, Jan 06, 2021 at 10:24:47AM +0100, Florian Obser wrote:
> On Wed, Jan 06, 2021 at 10:11:01AM +0100, Anton Lindqvist wrote:
> > Hi,
> > I have a unbound forward zone configured on my router for my $DAYJOB.
> > The address associated with the zone is only accessible when the router
> > is connected to a VPN. If the VPN connection is absent, trying to
> > resolve any domain that must be handled by the zone crashes unbound.
> > Turns out there's a missing NULL check in comm_point_send_udp_msg().
> > The same routine already has `if (addr) {} else {}' branches so I guess
> > protecting the call to log_addr() using the same conditional is
> > reasonable.
> >
> > Should this instead be upstreamed? Comments? OK?
>
> yes, please upstream it.
Pending pull request submitted:
https://github.com/NLnetLabs/unbound/pull/395
> Could you also do sbin/unwind/libunbound/util/netevent.c?
Sure, done.
fix opsfd parse.y shit/reduce conflicts
The dependon statement in ospfd parse.y introduces some troubles since it
holds an empty rule that then conflicts with optnl.
This diff changes dependon into dependon and dependonopt so that in the
place where it is optional dependonopt can be used and in the places where
it must not be optional it isn't. With this the shift/reduce conficts are
gone. While at it cleanup some other rules and use the same optnl idiom
for area and interface (it is the same one as used by bgpd).
Please test this with your configs to see if this causes any parse errors
(ospfd -n should be enough for this).
--
:wq Claudio
Index: parse.y
===
RCS file: /cvs/src/usr.sbin/ospfd/parse.y,v
retrieving revision 1.101
diff -u -p -r1.101 parse.y
--- parse.y 29 Dec 2020 19:44:47 - 1.101
+++ parse.y 6 Jan 2021 10:10:23 -
@@ -144,7 +144,7 @@ typedef struct {
%token NUMBER
%typeyesno no optlist optlist_l option demotecount msec
%typedeadtime
-%typestring dependon
+%typestring dependon dependonopt
%typeredistribute
%typeareaid
@@ -297,7 +297,7 @@ conf_main : ROUTERID STRING {
;
-redistribute : no REDISTRIBUTE NUMBER '/' NUMBER optlist dependon {
+redistribute : no REDISTRIBUTE NUMBER '/' NUMBER optlist dependonopt {
struct redistribute *r;
if ((r = calloc(1, sizeof(*r))) == NULL)
@@ -323,7 +323,7 @@ redistribute: no REDISTRIBUTE NUMBER '/
free($7);
$$ = r;
}
- | no REDISTRIBUTE STRING optlist dependon {
+ | no REDISTRIBUTE STRING optlist dependonopt {
struct redistribute *r;
if ((r = calloc(1, sizeof(*r))) == NULL)
@@ -426,8 +426,10 @@ option : METRIC NUMBER {
}
;
-dependon : /* empty */ { $$ = NULL; }
- | DEPEND ON STRING {
+dependonopt: /* empty */ { $$ = NULL; }
+ | dependon
+
+dependon : DEPEND ON STRING {
struct in_addr addr;
struct kif *kif;
@@ -599,7 +601,7 @@ area: AREA areaid {
memcpy(, defs, sizeof(areadefs));
md_list_copy(_list, >md_list);
defs =
- } '{' optnl areaopts_l '}' {
+ } '{' optnl areaopts_l optnl '}' {
area = NULL;
md_list_clr(>md_list);
defs =
@@ -627,8 +629,8 @@ areaid : NUMBER {
}
;
-areaopts_l : areaopts_l areaoptsl nl
- | areaoptsl optnl
+areaopts_l : areaopts_l nl areaoptsl
+ | areaoptsl
;
areaoptsl : interface
@@ -739,13 +741,13 @@ interface : INTERFACE STRING {
}
;
-interface_block: '{' optnl interfaceopts_l '}'
+interface_block: '{' optnl interfaceopts_l optnl '}'
| '{' optnl '}'
- |
+ | /* empty */
;
-interfaceopts_l: interfaceopts_l interfaceoptsl nl
- | interfaceoptsl optnl
+interfaceopts_l: interfaceopts_l nl interfaceoptsl
+ | interfaceoptsl
;
interfaceoptsl : PASSIVE { iface->passive = 1; }
Extend IP_ADD_MEMBERSHIP to support struct ip_mreqn
Linux and FreeBSD both support the use of struct ip_mreqn in
IP_ADD_MEMBERSHIP and IP_DROP_MEMBERSHIP. This struct adds one more field
to pass an interface index to the kernel (instead of using the IP
address).
struct ip_mreqn {
struct in_addr imr_multiaddr; /* IP multicast address of group */
struct in_addr imr_address;/* local IP address of interface */
int imr_ifindex;/* interface index */
};
So if imr_ifindex is not 0 then this value is used to define the outgoing
interface instead of doing a lookup with imr_address.
This is something I want to use in ospfd(8) to support unnumbered
interfaces (or actually point-to-point interfaces using the same source
IP).
--
:wq Claudio
Index: net/if_gre.c
===
RCS file: /cvs/src/sys/net/if_gre.c,v
retrieving revision 1.163
diff -u -p -r1.163 if_gre.c
--- net/if_gre.c12 Dec 2020 11:49:02 - 1.163
+++ net/if_gre.c6 Jan 2021 08:31:46 -
@@ -3640,7 +3640,7 @@ nvgre_up(struct nvgre_softc *sc)
switch (tunnel->t_af) {
case AF_INET:
- inm = in_addmulti(>t_dst4, ifp0);
+ inm = in_addmulti(>t_dst4, ifp0->if_index);
if (inm == NULL) {
error = ECONNABORTED;
goto remove_ucast;
Index: net/if_pfsync.c
===
RCS file: /cvs/src/sys/net/if_pfsync.c,v
retrieving revision 1.280
diff -u -p -r1.280 if_pfsync.c
--- net/if_pfsync.c 4 Jan 2021 12:48:27 - 1.280
+++ net/if_pfsync.c 6 Jan 2021 08:31:46 -
@@ -1424,7 +1424,7 @@ pfsyncioctl(struct ifnet *ifp, u_long cm
addr.s_addr = INADDR_PFSYNC_GROUP;
if ((imo->imo_membership[0] =
- in_addmulti(, sifp)) == NULL) {
+ in_addmulti(, sifp->if_index)) == NULL) {
sc->sc_sync_ifidx = 0;
return (ENOBUFS);
}
Index: net/if_vxlan.c
===
RCS file: /cvs/src/sys/net/if_vxlan.c,v
retrieving revision 1.81
diff -u -p -r1.81 if_vxlan.c
--- net/if_vxlan.c 21 Aug 2020 22:59:27 - 1.81
+++ net/if_vxlan.c 6 Jan 2021 08:31:46 -
@@ -274,7 +274,7 @@ vxlan_multicast_join(struct ifnet *ifp,
return (EADDRNOTAVAIL);
if ((imo->imo_membership[0] =
- in_addmulti(>sin_addr, mifp)) == NULL)
+ in_addmulti(>sin_addr, mifp->if_index)) == NULL)
return (ENOBUFS);
imo->imo_num_memberships++;
Index: netinet/in.c
===
RCS file: /cvs/src/sys/netinet/in.c,v
retrieving revision 1.170
diff -u -p -r1.170 in.c
--- netinet/in.c27 May 2020 11:19:28 - 1.170
+++ netinet/in.c6 Jan 2021 08:31:46 -
@@ -730,7 +730,7 @@ in_ifinit(struct ifnet *ifp, struct in_i
struct in_addr addr;
addr.s_addr = INADDR_ALLHOSTS_GROUP;
- ia->ia_allhosts = in_addmulti(, ifp);
+ ia->ia_allhosts = in_addmulti(, ifp->if_index);
}
out:
@@ -847,10 +847,15 @@ in_broadcast(struct in_addr in, u_int rt
* Add an address to the list of IP multicast addresses for a given interface.
*/
struct in_multi *
-in_addmulti(struct in_addr *ap, struct ifnet *ifp)
+in_addmulti(struct in_addr *ap, unsigned int ifidx)
{
struct in_multi *inm;
struct ifreq ifr;
+ struct ifnet *ifp;
+
+ ifp = if_get(ifidx);
+ if (ifp == NULL)
+ return (NULL);
/*
* See if address already in list.
@@ -867,14 +872,16 @@ in_addmulti(struct in_addr *ap, struct i
* and link it into the interface's multicast list.
*/
inm = malloc(sizeof(*inm), M_IPMADDR, M_NOWAIT | M_ZERO);
- if (inm == NULL)
+ if (inm == NULL) {
+ if_put(ifp);
return (NULL);
+ }
inm->inm_sin.sin_len = sizeof(struct sockaddr_in);
inm->inm_sin.sin_family = AF_INET;
inm->inm_sin.sin_addr = *ap;
inm->inm_refcnt = 1;
- inm->inm_ifidx = ifp->if_index;
+ inm->inm_ifidx = ifidx;
inm->inm_ifma.ifma_addr = sintosa(>inm_sin);
/*
@@ -884,6 +891,7 @@ in_addmulti(struct in_addr *ap, struct i
memset(, 0, sizeof(ifr));
memcpy(_addr, >inm_sin, sizeof(inm->inm_sin));
if ((*ifp->if_ioctl)(ifp, SIOCADDMULTI,(caddr_t)) != 0) {
+ if_put(ifp);
free(inm, M_IPMADDR, sizeof(*inm));
return (NULL);
}
@@ -891,12 +899,14 @@
Re: unbound: missing null check
On Wed, Jan 06, 2021 at 10:11:01AM +0100, Anton Lindqvist wrote:
> Hi,
> I have a unbound forward zone configured on my router for my $DAYJOB.
> The address associated with the zone is only accessible when the router
> is connected to a VPN. If the VPN connection is absent, trying to
> resolve any domain that must be handled by the zone crashes unbound.
> Turns out there's a missing NULL check in comm_point_send_udp_msg().
> The same routine already has `if (addr) {} else {}' branches so I guess
> protecting the call to log_addr() using the same conditional is
> reasonable.
>
> Should this instead be upstreamed? Comments? OK?
yes, please upstream it.
Could you also do sbin/unwind/libunbound/util/netevent.c?
OK florian
>
> Index: util/netevent.c
> ===
> RCS file: /cvs/src/usr.sbin/unbound/util/netevent.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 netevent.c
> --- util/netevent.c 10 Dec 2020 21:44:58 - 1.27
> +++ util/netevent.c 6 Jan 2021 09:03:59 -
> @@ -379,8 +379,9 @@ comm_point_send_udp_msg(struct comm_poin
> if(!udp_send_errno_needs_log(addr, addrlen))
> return 0;
> verbose(VERB_OPS, "sendto failed: %s", sock_strerror(errno));
> - log_addr(VERB_OPS, "remote address is",
> - (struct sockaddr_storage*)addr, addrlen);
> + if(addr)
> + log_addr(VERB_OPS, "remote address is",
> + (struct sockaddr_storage*)addr, addrlen);
> return 0;
> } else if((size_t)sent != sldns_buffer_remaining(packet)) {
> log_err("sent %d in place of %d bytes",
>
--
I'm not entirely sure you are real.
unbound: missing null check
Hi,
I have a unbound forward zone configured on my router for my $DAYJOB.
The address associated with the zone is only accessible when the router
is connected to a VPN. If the VPN connection is absent, trying to
resolve any domain that must be handled by the zone crashes unbound.
Turns out there's a missing NULL check in comm_point_send_udp_msg().
The same routine already has `if (addr) {} else {}' branches so I guess
protecting the call to log_addr() using the same conditional is
reasonable.
Should this instead be upstreamed? Comments? OK?
Index: util/netevent.c
===
RCS file: /cvs/src/usr.sbin/unbound/util/netevent.c,v
retrieving revision 1.27
diff -u -p -r1.27 netevent.c
--- util/netevent.c 10 Dec 2020 21:44:58 - 1.27
+++ util/netevent.c 6 Jan 2021 09:03:59 -
@@ -379,8 +379,9 @@ comm_point_send_udp_msg(struct comm_poin
if(!udp_send_errno_needs_log(addr, addrlen))
return 0;
verbose(VERB_OPS, "sendto failed: %s", sock_strerror(errno));
- log_addr(VERB_OPS, "remote address is",
- (struct sockaddr_storage*)addr, addrlen);
+ if(addr)
+ log_addr(VERB_OPS, "remote address is",
+ (struct sockaddr_storage*)addr, addrlen);
return 0;
} else if((size_t)sent != sldns_buffer_remaining(packet)) {
log_err("sent %d in place of %d bytes",
