linux-kernel - Re: [PATCH] selinux: Use pr

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]

Date:   Mon, 2 Jul 2018 21:15:49 -0400
From:   Paul Moore <paul@...l-moore.com>
To:     joe@...ches.com
Cc:     Stephen Smalley <sds@...ho.nsa.gov>,
        Eric Paris <eparis@...isplace.org>, peter.enderborg@...y.com,
        selinux@...ho.nsa.gov, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] selinux: Use pr_fmt to prefix "SELinux: "

On Mon, Jul 2, 2018 at 5:31 PM Joe Perches <joe@...ches.com> wrote:
> On Mon, 2018-07-02 at 16:51 -0400, Paul Moore wrote:
> > On Wed, Jun 20, 2018 at 2:39 AM Joe Perches <joe@...ches.com> wrote:
> > > pr_fmt can be used with the pr_<level> macros to prefix
> > > arbitrary content to logging messages.
> > >
> > > So add '#define pr_fmt(fmt) "SELinux: " fmt' to selinux files
> > > that use pr_<level> and remove embedded "SELinux: " prefixes
> > > from the format strings.
>
> []
> > > @@ -1644,13 +1640,12 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
> > >
> > >                                 if (rc == -EINVAL) {
> > >                                         if (printk_ratelimit())
> > > -                                               pr_notice("SELinux: inode=%lu on dev=%s was found to have an invalid "
> > > -                                                       "context=%s.  This indicates you may need to relabel the inode or the "
> > > -                                                       "filesystem in question.\n", ino, dev, context);
> > > +                                               pr_notice("inode=%lu on dev=%s was found to have an invalid context=%s.  This indicates you may need to relabel the inode or the filesystem in question.\n",
> > > +                                                         ino, dev, context);
> >
> > Please split up lines like this.  I realize that there isn't much room
> > left, but this message wraps a silly amount in my 80-char terminal; if
> > you need to wrap, please limit it to a word or two.
> >
> > To stop the argument before it starts, I don't care what checkpatch.pl
> > says about splitting printk format strings like this.
>
> I do, so does Linus.  It's also specified in CodingStyle.
>
> 2) Breaking long lines and strings
> ----------------------------------
> []
> never break user-visible strings such as
> printk messages, because that breaks the ability to grep for them.
> ----------------------------------
>
> Likely this would also use pr_notice_ratelimited(etc...)
> to reduce indentation a bit too.
>
> Probably these should be output on 2 lines as the output
> dmesg line length is extremely long.

That line is too long.  Split it please.

> > What tree did you base your patch on?
>
> next-20180619 on the day it was sent.
>
> > Please base SELinux patches
> > either on the SELinux tree or Linus' tree.  The SELinux tree can be
> > found at the links below:
> >
> > * git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
> > * https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git
>
> It's almost always better to use selinux/next.
> Why should this patch be different?

Yes, that's what I said, please base your patch against the selinux/next branch.

-- 
paul moore
www.paul-moore.com