linux-kernel - Re: [PATCH 67/68] TTY: isdn/gigaset, do not set tty->driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4F54F3E8.1070404@suse.cz>
Date:	Mon, 05 Mar 2012 18:12:08 +0100
From:	Jiri Slaby <jslaby@...e.cz>
To:	Tilman Schmidt <tilman@...p.cc>
CC:	Jiri Slaby <jirislaby@...il.com>, gregkh@...uxfoundation.org,
	alan@...ux.intel.com, linux-serial@...r.kernel.org,
	linux-kernel@...r.kernel.org, Hansjoerg Lipp <hjlipp@....de>,
	gigaset307x-common@...ts.sourceforge.net
Subject: Re: [PATCH 67/68] TTY: isdn/gigaset, do not set tty->driver_data
 to NULL

On 03/05/2012 06:05 PM, Tilman Schmidt wrote:
> Am 05.03.2012 14:52, schrieb Jiri Slaby:
>> Close the window in open where driver_data is reset to NULL on
>> each open. It could cause other processes to get invalid retval
>> from the tty->ops operations because of the checks all over the
>> code.
> 
>> With this change we may do other cleanups. Now, the only valid
>> check for tty->driver_data != NULL is in close. This can happen
>> only if open fails at gigaset_get_cs_by_tty or try_module_get.
>> The rest of checks in various tty->ops->* are invalid as
>> driver_data cannot be NULL there. The same holds for
>> cs->open_count. So remove them.
> 
> Thanks for that nice cleanup. It's most welcome. Just one question
> and a small nit:
> 
>> --- a/drivers/isdn/gigaset/interface.c +++
>> b/drivers/isdn/gigaset/interface.c
> [...]
>> @@ -178,12 +176,11 @@ static int if_open(struct tty_struct *tty,
>> struct file *filp)
> 
>> static void if_close(struct tty_struct *tty, struct file *filp) 
>> { -	struct cardstate *cs; +	struct cardstate *cs =
>> tty->driver_data; unsigned long flags;
> 
>> -	cs = (struct cardstate *) tty->driver_data; -	if (!cs) { -
>> pr_err("%s: no cardstate\n", __func__); +	if (!cs) { /* happens
>> if we didn't find cs in open */ +		printk(KERN_DEBUG "%s: no
>> cardstate\n", __func__); return; }
> 
> 
> Why are you downgrading the error message from KERN_ERR to
> KERN_DEBUG here? I would think that condition would warrant a
> message with KERN_ERR severity. Also, the driver does KERN_DEBUG
> output uniformly through the gig_dbg macro, so if you are sure it
> should be turned into a debug message then please write it as
> 
> gig_dbg(DEBUG_IF, "%s: no cardstate", __func__);
> 
> like four lines later.

<citing myself from the commit log>
Now, the only valid check for tty->driver_data != NULL is in close.
This can happen only if open fails at gigaset_get_cs_by_tty or
try_module_get.
</citing>

I.e. when the module behind the device is going away, driver_data can
be NULL. In that case we don't want to threaten the user by ERR messages.

You are maybe right, that we should switch it to gig_dbg or remove the
print completely (as it is a legitimate path). I'll wait until the
patches settles down a bit and fix it. If I, by a chance, forget to do
so, poke me or feel free to do it yourself ;).

thanks,
-- 
js
suse labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/