Re: check for device_lookup result in vscsi
On Fri, May 10, 2013 at 6:02 AM, Mike Belopuhov wrote: > my intention here is very simple: there's a way you should call > device_lookup and everyone has to fulfill it's part of the contract. > all our devices do, vscsi doesn't. what's the reason for it to be > one of a kind? I'm ok with adding a check, though I don't think it should really be necessary either. Just a thought: What about adding them as KASSERT()s? Why doesn't your diff include vscsiclose()?
Re: check for device_lookup result in vscsi
On Fri, May 10, 2013 at 10:35 -0700, Matthew Dempsky wrote:
> On Fri, May 10, 2013 at 6:02 AM, Mike Belopuhov wrote:
> > my intention here is very simple: there's a way you should call
> > device_lookup and everyone has to fulfill it's part of the contract.
> > all our devices do, vscsi doesn't. what's the reason for it to be
> > one of a kind?
>
> I'm ok with adding a check, though I don't think it should really be
> necessary either.
>
> Just a thought: What about adding them as KASSERT()s?
>
i don't have a strong opinion about that.
> Why doesn't your diff include vscsiclose()?
missed it.
diff --git sys/dev/vscsi.c sys/dev/vscsi.c
index 3da371c..a8f2c5b 100644
--- sys/dev/vscsi.c
+++ sys/dev/vscsi.c
@@ -296,6 +296,9 @@ vscsiioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
int read = 0;
int err = 0;
+ if (sc == NULL)
+ return (ENXIO);
+
rw_enter_write(&sc->sc_ioc_lock);
switch (cmd) {
@@ -476,6 +479,9 @@ vscsipoll(dev_t dev, int events, struct proc *p)
struct vscsi_softc *sc = DEV2SC(dev);
int revents = 0;
+ if (sc == NULL)
+ return (ENXIO);
+
if (events & (POLLIN | POLLRDNORM)) {
mtx_enter(&sc->sc_state_mtx);
if (!TAILQ_EMPTY(&sc->sc_ccb_i2t))
@@ -494,9 +500,14 @@ vscsipoll(dev_t dev, int events, struct proc *p)
int
vscsikqfilter(dev_t dev, struct knote *kn)
-{
+{
struct vscsi_softc *sc = DEV2SC(dev);
- struct klist *klist = &sc->sc_sel.si_note;
+ struct klist *klist;
+
+ if (sc == NULL)
+ return (ENXIO);
+
+ klist = &sc->sc_sel.si_note;
switch (kn->kn_filter) {
case EVFILT_READ:
@@ -548,6 +559,9 @@ vscsiclose(dev_t dev, int flags, int mode, struct proc *p)
struct vscsi_softc *sc = DEV2SC(dev);
struct vscsi_ccb*ccb;
+ if (sc == NULL)
+ return (ENXIO);
+
mtx_enter(&sc->sc_state_mtx);
KASSERT(sc->sc_state == VSCSI_S_RUNNING);
sc->sc_state = VSCSI_S_CONFIG;
Re: check for device_lookup result in vscsi
On 10 May 2013 14:57, Gerhard Roth wrote: > Mike, > > but it does check in vscsiopen(). Hence no userland program should be > able to call vscsiioctl() for a non-existant device because the open() > already failed. At least that's true as long as vscsi devices can't > disappear during run-time. > > Gerhard > my intention here is very simple: there's a way you should call device_lookup and everyone has to fulfill it's part of the contract. all our devices do, vscsi doesn't. what's the reason for it to be one of a kind?
Re: check for device_lookup result in vscsi
Mike,
but it does check in vscsiopen(). Hence no userland program should be
able to call vscsiioctl() for a non-existant device because the open()
already failed. At least that's true as long as vscsi devices can't
disappear during run-time.
Gerhard
On Fri, 10 May 2013 14:44:39 +0200 Mike Belopuhov wrote:
> On Fri, May 03, 2013 at 16:19 +0200, Mike Belopuhov wrote:
> > hi,
> >
> > while looking for the device_unref bugs, i found that
> > vscsi doesn't check if device_lookup has returned a
> > valid return value.
> >
> > ok?
> >
>
> anyone?
>
> > diff --git sys/dev/vscsi.c sys/dev/vscsi.c
> > index 3da371c..db65642 100644
> > --- sys/dev/vscsi.c
> > +++ sys/dev/vscsi.c
> > @@ -296,6 +296,9 @@ vscsiioctl(dev_t dev, u_long cmd, caddr_t addr, int
> > flags, struct proc *p)
> > int read = 0;
> > int err = 0;
> >
> > + if (sc == NULL)
> > + return (ENXIO);
> > +
> > rw_enter_write(&sc->sc_ioc_lock);
> >
> > switch (cmd) {
> > @@ -476,6 +479,9 @@ vscsipoll(dev_t dev, int events, struct proc *p)
> > struct vscsi_softc *sc = DEV2SC(dev);
> > int revents = 0;
> >
> > + if (sc == NULL)
> > + return (ENXIO);
> > +
> > if (events & (POLLIN | POLLRDNORM)) {
> > mtx_enter(&sc->sc_state_mtx);
> > if (!TAILQ_EMPTY(&sc->sc_ccb_i2t))
> > @@ -494,9 +500,14 @@ vscsipoll(dev_t dev, int events, struct proc *p)
> >
> > int
> > vscsikqfilter(dev_t dev, struct knote *kn)
> > -{
> > +{
> > struct vscsi_softc *sc = DEV2SC(dev);
> > - struct klist *klist = &sc->sc_sel.si_note;
> > + struct klist *klist;
> > +
> > + if (sc == NULL)
> > + return (ENXIO);
> > +
> > + klist = &sc->sc_sel.si_note;
> >
> > switch (kn->kn_filter) {
> > case EVFILT_READ:
>
Re: check for device_lookup result in vscsi
On Fri, May 03, 2013 at 16:19 +0200, Mike Belopuhov wrote:
> hi,
>
> while looking for the device_unref bugs, i found that
> vscsi doesn't check if device_lookup has returned a
> valid return value.
>
> ok?
>
anyone?
> diff --git sys/dev/vscsi.c sys/dev/vscsi.c
> index 3da371c..db65642 100644
> --- sys/dev/vscsi.c
> +++ sys/dev/vscsi.c
> @@ -296,6 +296,9 @@ vscsiioctl(dev_t dev, u_long cmd, caddr_t addr, int
> flags, struct proc *p)
> int read = 0;
> int err = 0;
>
> + if (sc == NULL)
> + return (ENXIO);
> +
> rw_enter_write(&sc->sc_ioc_lock);
>
> switch (cmd) {
> @@ -476,6 +479,9 @@ vscsipoll(dev_t dev, int events, struct proc *p)
> struct vscsi_softc *sc = DEV2SC(dev);
> int revents = 0;
>
> + if (sc == NULL)
> + return (ENXIO);
> +
> if (events & (POLLIN | POLLRDNORM)) {
> mtx_enter(&sc->sc_state_mtx);
> if (!TAILQ_EMPTY(&sc->sc_ccb_i2t))
> @@ -494,9 +500,14 @@ vscsipoll(dev_t dev, int events, struct proc *p)
>
> int
> vscsikqfilter(dev_t dev, struct knote *kn)
> -{
> +{
> struct vscsi_softc *sc = DEV2SC(dev);
> - struct klist *klist = &sc->sc_sel.si_note;
> + struct klist *klist;
> +
> + if (sc == NULL)
> + return (ENXIO);
> +
> + klist = &sc->sc_sel.si_note;
>
> switch (kn->kn_filter) {
> case EVFILT_READ:
check for device_lookup result in vscsi
hi,
while looking for the device_unref bugs, i found that
vscsi doesn't check if device_lookup has returned a
valid return value.
ok?
diff --git sys/dev/vscsi.c sys/dev/vscsi.c
index 3da371c..db65642 100644
--- sys/dev/vscsi.c
+++ sys/dev/vscsi.c
@@ -296,6 +296,9 @@ vscsiioctl(dev_t dev, u_long cmd, caddr_t addr, int flags,
struct proc *p)
int read = 0;
int err = 0;
+ if (sc == NULL)
+ return (ENXIO);
+
rw_enter_write(&sc->sc_ioc_lock);
switch (cmd) {
@@ -476,6 +479,9 @@ vscsipoll(dev_t dev, int events, struct proc *p)
struct vscsi_softc *sc = DEV2SC(dev);
int revents = 0;
+ if (sc == NULL)
+ return (ENXIO);
+
if (events & (POLLIN | POLLRDNORM)) {
mtx_enter(&sc->sc_state_mtx);
if (!TAILQ_EMPTY(&sc->sc_ccb_i2t))
@@ -494,9 +500,14 @@ vscsipoll(dev_t dev, int events, struct proc *p)
int
vscsikqfilter(dev_t dev, struct knote *kn)
-{
+{
struct vscsi_softc *sc = DEV2SC(dev);
- struct klist *klist = &sc->sc_sel.si_note;
+ struct klist *klist;
+
+ if (sc == NULL)
+ return (ENXIO);
+
+ klist = &sc->sc_sel.si_note;
switch (kn->kn_filter) {
case EVFILT_READ:
