linux-kernel - Re: [PATCH v2 3/3] tty: n_gsm: add parameter negotiation support

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] [day] [month] [year] [list]

Message-ID: <Y2C51txUYBGv53j+@kroah.com>
Date:   Tue, 1 Nov 2022 07:16:54 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     "Starke, Daniel" <daniel.starke@...mens.com>
Cc:     "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "jirislaby@...nel.org" <jirislaby@...nel.org>,
        "ilpo.jarvinen@...ux.intel.com" <ilpo.jarvinen@...ux.intel.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/3] tty: n_gsm: add parameter negotiation support

On Mon, Oct 31, 2022 at 01:26:50PM +0000, Starke, Daniel wrote:
> Thank you for your review.
> 
> > > +		pr_err("%s: unsupported frame type %d\n", __func__,
> > > +		       dlci->ftype);
> >
> > This needs to be dev_err(), right?
> 
> There is no place within this driver that uses dev_*() at the moment except
> for the timeout function of the network stack (gsm_mux_net_tx_timeout()).
> I do not mind either way, but I would prefer a consistent variant within
> this driver. Therefore, I suggest switching from pr_*() to dev_*() in a
> separate patch.

Yes, that's a good idea.

> > And why is it not just dev_dbg()/
> 
> I used pr_err() instead of pr_dbg() due to the fact that this mismatch will
> most likely make it impossible to use the n_gsm driver with the connected
> device as it stands. I am okay to replace it with pr_info() though.
> 
> > > +		pr_err("%s: unsupported adaption %d\n", __func__,
> > > +		       dlci->adaption);
> >
> > Again, dev_dbg()?
> >
> > Do not yet userspace, or external devices, spam kernel logs with
> > messages.
> 
> These are related to the user API. Therefore, I do not mind changing these
> to debug level.

Please do, userspace should not be able to spam kernel logs, bad things
can happen if that is possible.

> > And never use __func__ in a dev_dbg() call, it's there automatically.
> 
> I could not find a hint that __func__ is included in dev_dbg(). What is
> included is the subsystem name and the device name but not the function
> name within the driver according to include/linux/dev_printk.h. Other
> device drivers like usb/dwc2/core.c also include __func__ here. But it
> appears to be possible to automate this by re-defining dev_fmt().

You can dynamically add the function location at runtime when using
pr_dbg() or dev_dbg(), see the documentation.  So any addition of
__func__ in a string for those calls is just wrong and should be
removed.

Can you fix this up to not spam the log for errors (move to debug
level), and resend the updated series?

thanks,

greg k-h