netdev - Re: [PATCH] net: net_cls: remove a NULL check for css_cls

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: <CAJFZqHxzNFzg8hBMWc3DS3Nn1hdVxQ9cBhPSy4uL2b6ckCmbtg@mail.gmail.com>
Date:   Wed, 25 Apr 2018 16:35:02 +0800
From:   Li RongQing <roy.qing.li@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     lirongqing@...du.com, netdev@...r.kernel.org
Subject: Re: [PATCH] net: net_cls: remove a NULL check for css_cls_state

On 4/20/18, David Miller <davem@...emloft.net> wrote:
> From: Li RongQing <lirongqing@...du.com>
> Date: Thu, 19 Apr 2018 12:59:21 +0800
>
>> The input of css_cls_state() is impossible to NULL except
>> cgrp_css_online, so simplify it
>>
>> Signed-off-by: Li RongQing <lirongqing@...du.com>
>
> I don't view this as an improvement.  Just let the helper always check
> NULL and that way there are less situations to audit.
>
css_cls_state maybe return NULL, but nearly no places check the return
value with NULL, this seems unreadable.

net/core/netclassid_cgroup.c:27:        return
css_cls_state(task_css_check(p, net_cls_cgrp_id,
net/core/netclassid_cgroup.c:46:        struct cgroup_cls_state *cs =
css_cls_state(css);
net/core/netclassid_cgroup.c:47:        struct cgroup_cls_state
*parent = css_cls_state(css->parent);
net/core/netclassid_cgroup.c:57:        kfree(css_cls_state(css));
net/core/netclassid_cgroup.c:82:                           (void
*)(unsigned long)css_cls_state(css)->classid);
net/core/netclassid_cgroup.c:89:        return css_cls_state(css)->classid;
net/core/netclassid_cgroup.c:95:        struct cgroup_cls_state *cs =
css_cls_state(css);

> And it's not like this is a critical fast path either.
>

I see css_cls_state will be called  when send packet if
CONFIG_NET_CLS_ACT and CONFIG_NET_EGRESS enabled, the calling stack is
like below:

css_cls_state
  task_cls_state
    task_get_classid
       cls_cgroup_classify
          tcf_classify
            sch_handle_egress
               __dev_queue_xmit
                        CONFIG_NET_CLS_ACT
                         CONFIG_NET_EGRESS

-RongQing






> I'm not applying this, sorry.
>