| Message ID | ed83fae92c8c56e659f59c9a3709c86bd3a7d0c4.1449570042.git.tilman@imap.cc |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Tilman, On di, 2015-12-08 at 12:00 +0100, Tilman Schmidt wrote: > Commit f34d7a5b changed tty->driver to tty->ops but left NULL checks (This makes checkpatch complain, but the correct commit description style is used in the Fixes: tag, so it's not a big deal.) > for tty->driver untouched. Fix. > > Signed-off-by: Tilman Schmidt <tilman@imap.cc> > Fixes: f34d7a5b7010 ("tty: The big operations rework") Should we backport this all the way to v2.6.32 (currently the oldest stable tree)? > diff --git a/drivers/isdn/gigaset/ser-gigaset.c > b/drivers/isdn/gigaset/ser-gigaset.c > index 375be50..d8771b5 100644 > --- a/drivers/isdn/gigaset/ser-gigaset.c > +++ b/drivers/isdn/gigaset/ser-gigaset.c > @@ -67,7 +67,7 @@ static int write_modem(struct cardstate *cs) > struct sk_buff *skb = bcs->tx_skb; > int sent = -EOPNOTSUPP; > > - if (!tty || !tty->driver || !skb) > + if (!tty || !tty->ops || !skb) > return -EINVAL; > > if (!skb->len) { > @@ -109,7 +109,7 @@ static int send_cb(struct cardstate *cs) > unsigned long flags; > int sent = 0; > > - if (!tty || !tty->driver) > + if (!tty || !tty->ops) > return -EFAULT; > > cb = cs->cmdbuf; > @@ -432,7 +432,7 @@ static int gigaset_set_modem_ctrl(struct cardstate > *cs, unsigned old_state, > struct tty_struct *tty = cs->hw.ser->tty; > unsigned int set, clear; > > - if (!tty || !tty->driver || !tty->ops->tiocmset) > + if (!tty || !tty->ops || !tty->ops->tiocmset) > return -EINVAL; > set = new_state & ~old_state; > clear = old_state & ~new_state; It's pretty obvious that this should have been part of commit f34d7a5b7010 ("tty: The big operations rework"). That being said, these test puzzle me. It's not obvious why they're needed. Ie, can the null dereferences they try to catch really happen? But I can try to figure out that in the future, if I ever feel the urge to do so. Anyhow: Acked-by: Paul Bolle <pebolle@tiscali.nl> Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Should we backport this all the way to v2.6.32 (currently the oldest > stable tree)? We need to be able to explain how the case being tested can occur, then explain the situation in which it actually prevents a race condition. If nobody can do that then it shouldn't be backported because its change without value and just risk. The right fix as far as I can see is to remove the tests although WARN_ON() combined with your tty->ops change might be safer. > It's pretty obvious that this should have been part of commit > f34d7a5b7010 ("tty: The big operations rework"). That being said, these It ahould probably have been fixed around the same time or in one of the tty locking reviews, but drivers/isdn and net/irda weren't traditionally part of the general tty maintenance but handled separately/ > test puzzle me. It's not obvious why they're needed. Ie, can the null > dereferences they try to catch really happen? But I can try to figure > out that in the future, if I ever feel the urge to do so. Anyhow: > > Acked-by: Paul Bolle <pebolle@tiscali.nl> Nacked-by: Alan Cox <alan@linux.intel.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am 08.12.2015 um 23:16 schrieb One Thousand Gnomes: > The right fix as far as I can see is to remove the tests although > WARN_ON() combined with your tty->ops change might be safer. Feel free to submit a patch. >> It's pretty obvious that this should have been part of commit >> f34d7a5b7010 ("tty: The big operations rework"). That being said, these > > It ahould probably have been fixed around the same time or in one of the > tty locking reviews, but drivers/isdn and net/irda weren't traditionally > part of the general tty maintenance but handled separately/ Or just ignored. >> test puzzle me. It's not obvious why they're needed. Ie, can the null >> dereferences they try to catch really happen? But I can try to figure >> out that in the future, if I ever feel the urge to do so. Anyhow: >> >> Acked-by: Paul Bolle <pebolle@tiscali.nl> > > Nacked-by: Alan Cox <alan@linux.intel.com> So you feel it's better to maintain the current inconsistent state created by commit f34d7a5b? Please elaborate.
On Wed, 9 Dec 2015 11:45:57 +0100 Tilman Schmidt <tilman@imap.cc> wrote: > Am 08.12.2015 um 23:16 schrieb One Thousand Gnomes: > > The right fix as far as I can see is to remove the tests although > > WARN_ON() combined with your tty->ops change might be safer. > > Feel free to submit a patch. Will do later today. Want a patch on top of Paul's change or a single patch including both and crediting him ? > > tty locking reviews, but drivers/isdn and net/irda weren't traditionally > > part of the general tty maintenance but handled separately/ > > Or just ignored. Unfortunately at the time that seemed to happen a lot. > >> test puzzle me. It's not obvious why they're needed. Ie, can the null > >> dereferences they try to catch really happen? But I can try to figure > >> out that in the future, if I ever feel the urge to do so. Anyhow: > >> > >> Acked-by: Paul Bolle <pebolle@tiscali.nl> > > > > Nacked-by: Alan Cox <alan@linux.intel.com> > > So you feel it's better to maintain the current inconsistent state > created by commit f34d7a5b? Please elaborate. No I'd rather we didn't make it look magically better then forget about the mess in question. Alan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On wo, 2015-12-09 at 12:12 +0000, One Thousand Gnomes wrote: > On Wed, 9 Dec 2015 11:45:57 +0100 > Tilman Schmidt <tilman@imap.cc> wrote: > Want a patch on top of Paul's change or a single > patch including both and crediting him ? There's no change that can be attributed to me, I think. We're discussing a series submitted by Tilman. Confused, Paul Bolle -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/isdn/gigaset/ser-gigaset.c b/drivers/isdn/gigaset/ser-gigaset.c index 375be50..d8771b5 100644 --- a/drivers/isdn/gigaset/ser-gigaset.c +++ b/drivers/isdn/gigaset/ser-gigaset.c @@ -67,7 +67,7 @@ static int write_modem(struct cardstate *cs) struct sk_buff *skb = bcs->tx_skb; int sent = -EOPNOTSUPP; - if (!tty || !tty->driver || !skb) + if (!tty || !tty->ops || !skb) return -EINVAL; if (!skb->len) { @@ -109,7 +109,7 @@ static int send_cb(struct cardstate *cs) unsigned long flags; int sent = 0; - if (!tty || !tty->driver) + if (!tty || !tty->ops) return -EFAULT; cb = cs->cmdbuf; @@ -432,7 +432,7 @@ static int gigaset_set_modem_ctrl(struct cardstate *cs, unsigned old_state, struct tty_struct *tty = cs->hw.ser->tty; unsigned int set, clear; - if (!tty || !tty->driver || !tty->ops->tiocmset) + if (!tty || !tty->ops || !tty->ops->tiocmset) return -EINVAL; set = new_state & ~old_state; clear = old_state & ~new_state;
Commit f34d7a5b changed tty->driver to tty->ops but left NULL checks for tty->driver untouched. Fix. Signed-off-by: Tilman Schmidt <tilman@imap.cc> Fixes: f34d7a5b7010 ("tty: The big operations rework") --- drivers/isdn/gigaset/ser-gigaset.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)