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
| ||
|
Date: Fri, 29 Sep 2017 08:46:01 +0200 From: Simon Horman <simon.horman@...ronome.com> To: Cong Wang <xiyou.wangcong@...il.com> Cc: Linux Kernel Network Developers <netdev@...r.kernel.org>, Chris Mi <chrism@...lanox.com>, Jamal Hadi Salim <jhs@...atatu.com> Subject: Re: [Patch net-next] net_sched: use idr to allocate u32 filter handles On Thu, Sep 28, 2017 at 03:19:05PM -0700, Cong Wang wrote: > On Thu, Sep 28, 2017 at 12:34 AM, Simon Horman > <simon.horman@...ronome.com> wrote: > > Hi Cong, > > > > this looks like a nice enhancement to me. Did you measure any performance > > benefit from it. Perhaps it could be described in the changelog_ I also > > have a more detailed question below. > > No, I am inspired by commit c15ab236d69d, don't measure it. Perhaps it would be nice to note that in the changelog. > >> --- > >> net/sched/cls_u32.c | 108 ++++++++++++++++++++++++++++++++-------------------- > >> 1 file changed, 67 insertions(+), 41 deletions(-) > >> > >> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c > >> index 10b8d851fc6b..316b8a791b13 100644 > >> --- a/net/sched/cls_u32.c > >> +++ b/net/sched/cls_u32.c > >> @@ -46,6 +46,7 @@ > > > > ... > > > >> @@ -937,22 +940,33 @@ static int u32_change(struct net *net, struct sk_buff *in_skb, > >> return -EINVAL; > >> if (TC_U32_KEY(handle)) > >> return -EINVAL; > >> - if (handle == 0) { > >> - handle = gen_new_htid(tp->data); > >> - if (handle == 0) > >> - return -ENOMEM; > >> - } > >> ht = kzalloc(sizeof(*ht) + divisor*sizeof(void *), GFP_KERNEL); > >> if (ht == NULL) > >> return -ENOBUFS; > >> + if (handle == 0) { > >> + handle = gen_new_htid(tp->data, ht); > >> + if (handle == 0) { > >> + kfree(ht); > >> + return -ENOMEM; > >> + } > >> + } else { > >> + err = idr_alloc_ext(&tp_c->handle_idr, ht, NULL, > >> + handle, handle + 1, GFP_KERNEL); > >> + if (err) { > >> + kfree(ht); > >> + return err; > >> + } > > > > The above seems to check that handle is not already in use and mark it as > > in use. But I don't see that logic in the code prior to this patch. > > Am I missing something? If not perhaps this portion should be a separate > > patch or described in the changelog. > > The logic is in upper layer, tc_ctl_tfilter(). It tries to get a > filter by handle > (if non-zero), and errors out if we are creating a new filter with the same > handle. > > At the point you quote above, 'n' is already NULL and 'handle' is non-zero, > which means there is no existing filter has same handle, it is safe to just > mark it as in-use. Thanks for the clarification, that seems fine to me. Reviewed-by: Simon Horman <simon.horman@...ronome.com>
Powered by blists - more mailing lists