Re: [PATCH v3] selinux: lsm: fix a missing-check bug in selinux_sb_eat_lsm_opts()

[Paul Moore]

On Mon, Jun 3, 2019 at 3:23 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> On Sat, Jun 1, 2019 at 4:15 AM Gen Zhang <blackgod016574@xxxxxxxxx> wrote:
> > In selinux_sb_eat_lsm_opts(), 'arg' is allocated by kmemdup_nul(). It
> > returns NULL when fails. So 'arg' should be checked. And 'mnt_opts'
> > should be freed when error.
> >
> > Signed-off-by: Gen Zhang <blackgod016574@xxxxxxxxx>
> > Reviewed-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
>
> It looks like you're new to the kernel development community, so let
> me give you a bit of friendly advice for the future :)
>
> You don't need to repost the patch when people give you
> Acked-by/Reviewed-by/Tested-by (unless there is a different reason to
> respin/repost the patches). The maintainer goes over the replies when
> applying the final patch and adds Acked-by/Reviewed-by/... on his/her
> own.
>
> If you *do* need to respin a path for which you have received A/R/T,
> then you need to distinguish between two cases:
> 1. Only trivial changes to the patch (only fixed typos, edited commit
> message, removed empty line, etc. - for example, v1 -> v2 of this
> patch falls into this category) - in this case you can collect the
> A/R/T yourself and add them to the new version. This saves the
> maintainer and the reviewers from redundant work, since the patch is
> still semantically the same and the A/R/T from the last version still
> apply.
> 2. Non-trivial changes to the patch (as is the case for this patch) -
> in this case your patch needs to be reviewed again and you should
> disregard all A/R/T from the previous version. You can easily piss
> someone off if you add their Reviewed-by to a patch they haven't
> actually reviewed, so be careful ;-)

I want to stress Ondrej's last point.  Carrying over an
Acked-by/Reviewed-by/Tested-by tag if you make anything more than the
most trivial change in a patch is *very* bad, and will likely result
in a loss of trust between you and the maintainer.  If you are unsure,
drop the A/R/T tag, there is *much* less harm in asking someone to
re-review a patch than falsely tagging a patch as reviewed by someone
when you have made substantial changes.

I suspect you may have already read the
Documentation/process/submitting-patches.rst file, but if you haven't
it is worth reading.  It covers many of the things that are discussed
elsewhere.

If you aren't already, you should get in the habit of doing the
following for each patch you post to the mailing list:
1. Make sure it compiles cleanly, or at least doesn't introduce any
new compiler warnings/errors.
2. Run ./scripts/checkpatch.pl and fix as many problems as you can; a
patch can still be accepted with checkpatch warnings/errors (and some
maintainers might dislike some of checkpatch's decisions), but it
helps a lot if you can fix all those.
3. At the very least make sure your kernel changes boot, if you can,
run the associated subsystem's test (if they have one) to verify that
there are no regressions (the SELinux kernel test suite is here:
https://github.com/SELinuxProject/selinux-testsuite)

Lastly, when in doubt, you can always ask the mailing list; the
SELinux list is a pretty friendly place :)

-- 
paul moore
www.paul-moore.com