On Sat, Feb 13, 2021 at 10:45:17AM +0100, Claudio Jeker wrote:
> On Sat, Feb 13, 2021 at 10:26:48AM +0100, Marcus Glocker wrote:
> > On Sat, Feb 13, 2021 at 08:30:04AM +0100, Claudio Jeker wrote:
> >
> > > On Fri, Feb 12, 2021 at 10:59:05PM +0100, Jeremie Courreges-Anglas wrote:
> > > > On Wed, Feb 10 2021, Martin Pieuchot <m...@openbsd.org> wrote:
> > > >
> > > > [...]
> > > >
> > > > > Which fields is the new lock protecting? Why isn't the KERNEL_LOCK()
> > > > > enough?
> > > >
> > > > When I mentioned this potential lack of locking to Marcus, I was
> > > > mistaken. Some of the functions in video.c are called from syscalls
> > > > that are marked NOLOCK. But now that I have added some printf
> > > > debugging, I can see that the kernel lock is held in places where
> > > > I didn't expect it to be held (videoioctl, videoread, videoclose...).
> > > > So there's probably a layer of locking that I am missing.
> > > >
> > > > Marcus, sorry for my misleading diff. O:)
> > > >
> > > > *crawls back under his rock*
> > >
> > > Just because a syscall is marked NOLOCK does not mean that all of it will
> > > run without the KERNEL_LOCK. For example read and write are marked NOLOCK
> > > but the KERNEL_LOCK is grabbed later for all non-socket filedescriptors.
> > > This is why video(4) still runs with the KERNEL_LOCK.
> > > The NOLOCK in syscalls.master just tell the kernel to not grab the
> > > KERNEL_LOCK right at the start of the syscall.
> >
> > OK, that wasn't clear to me either! If Jeremie has some space left
> > under his rock, I would join :-)
> >
> > That basically means we need no extra locking in video(4) for this
> > implementation, right? For a test I replaced the rw_enter_write()
> > with KERNEL_ASSERT_LOCKED(), running a stream and multiple concurrent
> > ioctl programs, and all went fine.
> >
> > I would come up with a revised diff then for the device owner part.
> > Just doing some polishing together with Anton on that.
>
> For now this is OK. I have some diffs around which will allow unlocked
> read and write down to individual devices (I was experimenting with tun(4)
> and tap(4)). For ioctl() this will not happen any time soon. ioctl() is a
> world of pain. Maybe for devices like video(4) the call path will be
> manageable but for network devices it is just mess. The main issue is that
> ioctls work on multiple layers and sometimes the calls have layer
> violations and often multiple sleep points built into them.
Got it. Thanks for the explanation.
Here the adapted diff, including a lot of input from anton@.
I sprinkled KERNEL_ASSERT_LOCKED() in those functions where we require
locking. Not sure if we should keep them in?
Index: sys/dev/video.c
===================================================================
RCS file: /cvs/src/sys/dev/video.c,v
retrieving revision 1.48
diff -u -p -u -p -r1.48 video.c
--- sys/dev/video.c 31 Jan 2021 19:32:01 -0000 1.48
+++ sys/dev/video.c 13 Feb 2021 09:49:45 -0000
@@ -47,7 +47,8 @@ struct video_softc {
struct device *sc_dev; /* hardware device struct */
struct video_hw_if *hw_if; /* hardware interface */
char sc_dying; /* device detached */
- struct process *sc_owner; /* owner process */
+ struct process *sc_owner; /* owner process */
+ uint8_t sc_open; /* device opened */
int sc_fsize;
uint8_t *sc_fbuffer;
@@ -69,6 +70,8 @@ int videoactivate(struct device *, int);
int videoprint(void *, const char *);
void video_intr(void *);
+int video_stop(struct video_softc *);
+int video_claim(struct video_softc *, struct process *);
struct cfattach video_ca = {
sizeof(struct video_softc), videoprobe, videoattach,
@@ -122,6 +125,9 @@ videoopen(dev_t dev, int flags, int fmt,
{
int unit;
struct video_softc *sc;
+ int error = 0;
+
+ KERNEL_ASSERT_LOCKED();
unit = VIDEOUNIT(dev);
if (unit >= video_cd.cd_ndevs ||
@@ -129,38 +135,40 @@ videoopen(dev_t dev, int flags, int fmt,
sc->hw_if == NULL)
return (ENXIO);
- if (sc->sc_owner != NULL) {
- if (sc->sc_owner == p->p_p)
- return (0);
- else
- return (EBUSY);
- } else
- sc->sc_owner = p->p_p;
+ if (sc->sc_open) {
+ DPRINTF(("%s: device already open\n", __func__));
+ return (0);
+ } else {
+ sc->sc_open = 1;
+ DPRINTF(("%s: set device to open\n", __func__));
+ }
sc->sc_vidmode = VIDMODE_NONE;
sc->sc_frames_ready = 0;
if (sc->hw_if->open != NULL)
- return (sc->hw_if->open(sc->hw_hdl, flags, &sc->sc_fsize,
- sc->sc_fbuffer, video_intr, sc));
- else
- return (0);
+ error = sc->hw_if->open(sc->hw_hdl, flags, &sc->sc_fsize,
+ sc->sc_fbuffer, video_intr, sc);
+
+ return (error);
}
int
videoclose(dev_t dev, int flags, int fmt, struct proc *p)
{
struct video_softc *sc;
- int r = 0;
+ int error = 0;
- sc = video_cd.cd_devs[VIDEOUNIT(dev)];
+ KERNEL_ASSERT_LOCKED();
- if (sc->hw_if->close != NULL)
- r = sc->hw_if->close(sc->hw_hdl);
+ DPRINTF(("%s: last close\n", __func__));
- sc->sc_owner = NULL;
+ sc = video_cd.cd_devs[VIDEOUNIT(dev)];
+
+ sc->sc_open = 0;
+ error = video_stop(sc);
- return (r);
+ return (error);
}
int
@@ -170,6 +178,8 @@ videoread(dev_t dev, struct uio *uio, in
int unit, error;
size_t size;
+ KERNEL_ASSERT_LOCKED();
+
unit = VIDEOUNIT(dev);
if (unit >= video_cd.cd_ndevs ||
(sc = video_cd.cd_devs[unit]) == NULL)
@@ -181,6 +191,9 @@ videoread(dev_t dev, struct uio *uio, in
if (sc->sc_vidmode == VIDMODE_MMAP)
return (EBUSY);
+ if ((error = video_claim(sc, curproc->p_p)))
+ return (error);
+
/* start the stream if not already started */
if (sc->sc_vidmode == VIDMODE_NONE && sc->hw_if->start_read) {
error = sc->hw_if->start_read(sc->hw_hdl);
@@ -221,6 +234,8 @@ videoioctl(dev_t dev, u_long cmd, caddr_
struct v4l2_buffer *vb = (struct v4l2_buffer *)data;
int unit, error;
+ KERNEL_ASSERT_LOCKED();
+
unit = VIDEOUNIT(dev);
if (unit >= video_cd.cd_ndevs ||
(sc = video_cd.cd_devs[unit]) == NULL || sc->hw_if == NULL)
@@ -231,6 +246,31 @@ videoioctl(dev_t dev, u_long cmd, caddr_
error = EOPNOTSUPP;
switch (cmd) {
+ case VIDIOC_G_CTRL:
+ if (sc->hw_if->g_ctrl)
+ error = (sc->hw_if->g_ctrl)(sc->hw_hdl,
+ (struct v4l2_control *)data);
+ break;
+ case VIDIOC_S_CTRL:
+ if (sc->hw_if->s_ctrl)
+ error = (sc->hw_if->s_ctrl)(sc->hw_hdl,
+ (struct v4l2_control *)data);
+ break;
+ default:
+ error = (ENOTTY);
+ }
+ if (error != ENOTTY)
+ return (error);
+
+ if ((error = video_claim(sc, p->p_p)))
+ return (error);
+
+ /*
+ * The following IOCTLs can only be called by the device owner.
+ * For further shared IOCTLs please move it up.
+ */
+ error = EOPNOTSUPP;
+ switch (cmd) {
case VIDIOC_QUERYCAP:
if (sc->hw_if->querycap)
error = (sc->hw_if->querycap)(sc->hw_hdl,
@@ -326,6 +366,9 @@ videoioctl(dev_t dev, u_long cmd, caddr_
if (sc->hw_if->streamoff)
error = (sc->hw_if->streamoff)(sc->hw_hdl,
(int)*data);
+ if (!error)
+ /* Release device ownership and streaming buffers. */
+ video_stop(sc);
break;
case VIDIOC_TRY_FMT:
if (sc->hw_if->try_fmt)
@@ -337,16 +380,6 @@ videoioctl(dev_t dev, u_long cmd, caddr_
error = (sc->hw_if->queryctrl)(sc->hw_hdl,
(struct v4l2_queryctrl *)data);
break;
- case VIDIOC_G_CTRL:
- if (sc->hw_if->g_ctrl)
- error = (sc->hw_if->g_ctrl)(sc->hw_hdl,
- (struct v4l2_control *)data);
- break;
- case VIDIOC_S_CTRL:
- if (sc->hw_if->s_ctrl)
- error = (sc->hw_if->s_ctrl)(sc->hw_hdl,
- (struct v4l2_control *)data);
- break;
default:
error = (ENOTTY);
}
@@ -361,6 +394,8 @@ videopoll(dev_t dev, int events, struct
struct video_softc *sc;
int error, revents = 0;
+ KERNEL_ASSERT_LOCKED();
+
if (unit >= video_cd.cd_ndevs ||
(sc = video_cd.cd_devs[unit]) == NULL)
return (POLLERR);
@@ -368,6 +403,9 @@ videopoll(dev_t dev, int events, struct
if (sc->sc_dying)
return (POLLERR);
+ if ((error = video_claim(sc, p->p_p)))
+ return (error);
+
DPRINTF(("%s: events=0x%x\n", __func__, events));
if (events & (POLLIN | POLLRDNORM)) {
@@ -405,6 +443,8 @@ videommap(dev_t dev, off_t off, int prot
caddr_t p;
paddr_t pa;
+ KERNEL_ASSERT_LOCKED();
+
DPRINTF(("%s: off=%lld, prot=%d\n", __func__, off, prot));
unit = VIDEOUNIT(dev);
@@ -466,7 +506,9 @@ videokqfilter(dev_t dev, struct knote *k
{
int unit = VIDEOUNIT(dev);
struct video_softc *sc;
- int s;
+ int s, error;
+
+ KERNEL_ASSERT_LOCKED();
if (unit >= video_cd.cd_ndevs ||
(sc = video_cd.cd_devs[unit]) == NULL)
@@ -484,6 +526,9 @@ videokqfilter(dev_t dev, struct knote *k
return (EINVAL);
}
+ if ((error = video_claim(sc, curproc->p_p)))
+ return (error);
+
/*
* Start the stream in read() mode if not already started. If
* the user wanted mmap() mode, he should have called mmap()
@@ -537,6 +582,39 @@ video_intr(void *addr)
if (sc->sc_vidmode == VIDMODE_READ)
wakeup(sc);
selwakeup(&sc->sc_rsel);
+}
+
+int
+video_stop(struct video_softc *sc)
+{
+ int error = 0;
+
+ DPRINTF(("%s: stream close\n", __func__));
+
+ if (sc->hw_if->close != NULL)
+ error = sc->hw_if->close(sc->hw_hdl);
+
+ sc->sc_vidmode = VIDMODE_NONE;
+ sc->sc_frames_ready = 0;
+ sc->sc_owner = NULL;
+
+ return (error);
+}
+
+int
+video_claim(struct video_softc *sc, struct process *p)
+{
+ if (sc->sc_owner != NULL && sc->sc_owner != p) {
+ DPRINTF(("%s: already owned=%p\n", __func__, sc->sc_owner));
+ return (EBUSY);
+ }
+
+ if (sc->sc_owner == NULL) {
+ sc->sc_owner = p;
+ DPRINTF(("%s: new owner=%p\n", __func__, sc->sc_owner));
+ }
+
+ return (0);
}
int
