On Thu, 25 May 2006, Paul Moore wrote:
> This patch introduces a new kernel feature designed to support labeled
> networking protocols such as RIPSO and CIPSO. These protocols are required to
> interoperate with existing "trusted" operating systems such as Trusted
> Solaris.
A few initial comments.
- Did you decide that you definitely need to verify labels on fragments?
I can see the code's been added to do that, but wonder about a comment
made during earlier discussion that mislabeled fragments could only come
from a misbehaving trusted system. What is the threat model here?
- Can you explain how module loading and module refcounting for these
modules work? (e.g. what causes netlabel_cipso_v4 to be loaded, is it
always safe to unload if the refcount is zero?)
- What about user APIs for setting and retrieving labels?
- What about labeling of kernel-generated packets?
- Don't put #ifdef'd code into mainline code.
e.g. in net/ipv4/ip_fragment.c
+#ifdef CONFIG_NETLABEL_CIPSOV4
+ if (sopt->cipso) {
This needs to be a function which is compiled away as a static inline when
not selected. This stuff should have zero impact on the networking code
if not enabled.
- Try and add entries for security/selinux/nlmsgtab.c for the new Netlink
protocol.
- This does not follow normal kernel coding practices:
+ if (netlbl_netlink_init() != 0) {
+ netlbl_domhsh_exit();
+ return -ENOMEM;
This should be:
{
err = netlbl_netlink_init();
if (err)
goto err_domhsh;
...
err_domhsh:
netlbl_domhsh_exit();
return err;
}
i.e. a single error path when you have cleanups to perform, and
propagation of error codes.
- This kind of stuff should be removed before merging:
+static void __exit netlbl_exit(void)
+{
+ printk(KERN_INFO "NetLabel: Exiting\n");
+int netlbl_netlink_init(void)
+{
+ BUG_ON(netlbl_nl);
+int netlbl_netlink_exit(void)
+{
+ BUG_ON(!netlbl_nl);
Should the above two be marked __init and __exit? And why does the last
one return an int when the only possible return value is zero? (it needs
to return void).
- Why does this module have a version number?
+ printk(KERN_INFO "NetLabel: Initializing (v%s %s)\n",
+ NETLBL_VER_STR, NETLBL_VER_DATE);
--
James Morris
<[EMAIL PROTECTED]>
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
