| Message ID | 1381904114-29556-1-git-send-email-fan.du@windriver.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wednesday, October 16, 2013 02:15:14 PM Fan Du wrote: > Taking advantages of sadb_x_sec_ctx and xfrm_user_sec_ctx share the same > structure arrangement, rework pfkey_sadb2xfrm_user_sec_ctx by casting > sadb_x_sec_ctx into xfrm_user_sec_ctx with minor len fix. > > Then we can: > -Avoid kmalloc/free memory for xfrm_user_sec_ctx, sadb_x_sec_ctx would be > fine. > -Fix missing return value check bug for pfkey_compile_policy when > kmalloc fails > > Signed-off-by: Fan Du <fan.du@windriver.com> > --- > net/key/af_key.c | 33 +-------------------------------- > 1 file changed, 1 insertion(+), 32 deletions(-) > > diff --git a/net/key/af_key.c b/net/key/af_key.c > index 9d58537..c7d304d 100644 > --- a/net/key/af_key.c > +++ b/net/key/af_key.c > @@ -435,22 +435,9 @@ static inline int verify_sec_ctx_len(const void *p) > > static inline struct xfrm_user_sec_ctx *pfkey_sadb2xfrm_user_sec_ctx(const > struct sadb_x_sec_ctx *sec_ctx) { > - struct xfrm_user_sec_ctx *uctx = NULL; > - int ctx_size = sec_ctx->sadb_x_ctx_len; > - > - uctx = kmalloc((sizeof(*uctx)+ctx_size), GFP_KERNEL); > - > - if (!uctx) > - return NULL; > + struct xfrm_user_sec_ctx *uctx = (struct xfrm_user_sec_ctx *)sec_ctx; > > uctx->len = pfkey_sec_ctx_len(sec_ctx); > - uctx->exttype = sec_ctx->sadb_x_sec_exttype; > - uctx->ctx_doi = sec_ctx->sadb_x_ctx_doi; > - uctx->ctx_alg = sec_ctx->sadb_x_ctx_alg; > - uctx->ctx_len = sec_ctx->sadb_x_ctx_len; > - memcpy(uctx + 1, sec_ctx + 1, > - uctx->ctx_len); > - > return uctx; > } The fact that you are now changing sadb_x_sec_ctx->sadb_x_sec_len whenever pfkey_sadb2xfrm_user_sec_ctx() is called raises an eyebrow. Can you elaborate on why this is not a problem?
On 2013年10月16日 23:15, Paul Moore wrote: > On Wednesday, October 16, 2013 02:15:14 PM Fan Du wrote: >> Taking advantages of sadb_x_sec_ctx and xfrm_user_sec_ctx share the same >> structure arrangement, rework pfkey_sadb2xfrm_user_sec_ctx by casting >> sadb_x_sec_ctx into xfrm_user_sec_ctx with minor len fix. >> >> Then we can: >> -Avoid kmalloc/free memory for xfrm_user_sec_ctx, sadb_x_sec_ctx would be >> fine. >> -Fix missing return value check bug for pfkey_compile_policy when >> kmalloc fails >> >> Signed-off-by: Fan Du<fan.du@windriver.com> >> --- >> net/key/af_key.c | 33 +-------------------------------- >> 1 file changed, 1 insertion(+), 32 deletions(-) >> >> diff --git a/net/key/af_key.c b/net/key/af_key.c >> index 9d58537..c7d304d 100644 >> --- a/net/key/af_key.c >> +++ b/net/key/af_key.c >> @@ -435,22 +435,9 @@ static inline int verify_sec_ctx_len(const void *p) >> >> static inline struct xfrm_user_sec_ctx *pfkey_sadb2xfrm_user_sec_ctx(const >> struct sadb_x_sec_ctx *sec_ctx) { >> - struct xfrm_user_sec_ctx *uctx = NULL; >> - int ctx_size = sec_ctx->sadb_x_ctx_len; >> - >> - uctx = kmalloc((sizeof(*uctx)+ctx_size), GFP_KERNEL); >> - >> - if (!uctx) >> - return NULL; >> + struct xfrm_user_sec_ctx *uctx = (struct xfrm_user_sec_ctx *)sec_ctx; >> >> uctx->len = pfkey_sec_ctx_len(sec_ctx); >> - uctx->exttype = sec_ctx->sadb_x_sec_exttype; >> - uctx->ctx_doi = sec_ctx->sadb_x_ctx_doi; >> - uctx->ctx_alg = sec_ctx->sadb_x_ctx_alg; >> - uctx->ctx_len = sec_ctx->sadb_x_ctx_len; >> - memcpy(uctx + 1, sec_ctx + 1, >> - uctx->ctx_len); >> - >> return uctx; >> } > > The fact that you are now changing sadb_x_sec_ctx->sadb_x_sec_len whenever > pfkey_sadb2xfrm_user_sec_ctx() is called raises an eyebrow. Can you elaborate > on why this is not a problem? > Thanks for your attention, Paul. sadb_x_sec_ctx is extra headers passed down from user space, the usage of of this data structure falls down to one of pfkey_funcs function only for one time, more specifically speaking, it's only used by SELINUX for security checking for each operation. In other words, sadb_x_sec_ctx involves with a one shot business here. So the original codes seems do a lots of extra job which could easily be avoid using casting operation.
On Thu, Oct 17, 2013 at 09:34:53AM +0800, Fan Du wrote: > > > On 2013年10月16日 23:15, Paul Moore wrote: > > > >The fact that you are now changing sadb_x_sec_ctx->sadb_x_sec_len whenever > >pfkey_sadb2xfrm_user_sec_ctx() is called raises an eyebrow. Can you elaborate > >on why this is not a problem? > > > Thanks for your attention, Paul. > > sadb_x_sec_ctx is extra headers passed down from user space, the usage of > of this data structure falls down to one of pfkey_funcs function only for > one time, more specifically speaking, it's only used by SELINUX for security > checking for each operation. In other words, sadb_x_sec_ctx involves with a > one shot business here. So the original codes seems do a lots of extra job > which could easily be avoid using casting operation. > Since the selinux people have to live with that change in the fist place, I'd like to see an ack of one of the selinux maintainers before I take in into ipsec-next, Paul? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, October 17, 2013 11:51:48 AM Steffen Klassert wrote: > On Thu, Oct 17, 2013 at 09:34:53AM +0800, Fan Du wrote: > > On 2013年10月16日 23:15, Paul Moore wrote: > > >The fact that you are now changing sadb_x_sec_ctx->sadb_x_sec_len > > >whenever > > >pfkey_sadb2xfrm_user_sec_ctx() is called raises an eyebrow. Can you > > >elaborate on why this is not a problem? > > > > Thanks for your attention, Paul. > > > > sadb_x_sec_ctx is extra headers passed down from user space, the usage of > > of this data structure falls down to one of pfkey_funcs function only for > > one time, more specifically speaking, it's only used by SELINUX for > > security checking for each operation. In other words, sadb_x_sec_ctx > > involves with a one shot business here. So the original codes seems do a > > lots of extra job which could easily be avoid using casting operation. > > Since the selinux people have to live with that change in the fist place, > I'd like to see an ack of one of the selinux maintainers before I take > in into ipsec-next, Paul? Well, my earlier concern over modifying the length field probably isn't a major concern as was pointed out so I could maybe look the other way on that point. However, while looking a bit closer at the structs and how they are used, I noticed that PFKEY structs are all explicitly packed (sadb_x_sec_ctx) and the LSM struct is not (xfrm_user_sec_ctx). I'm not enough of a compiler guru across all the different architectures to know if this is significant or not given the structure, but it does make me pause. Any compiler gurus care to weigh in on this?
From: Fan Du <fan.du@windriver.com> Date: Wed, 16 Oct 2013 14:15:14 +0800 > Taking advantages of sadb_x_sec_ctx and xfrm_user_sec_ctx share the same > structure arrangement, rework pfkey_sadb2xfrm_user_sec_ctx by casting > sadb_x_sec_ctx into xfrm_user_sec_ctx with minor len fix. > > Then we can: > -Avoid kmalloc/free memory for xfrm_user_sec_ctx, sadb_x_sec_ctx would be fine. > -Fix missing return value check bug for pfkey_compile_policy when kmalloc fails > > Signed-off-by: Fan Du <fan.du@windriver.com> This isn't safe, one structure is packed and the other is not. Furthermore, unless there is some enormous gain (in this case there is not) losing the type checking by casting two data structures like this is undesirable. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013年10月19日 03:58, David Miller wrote: > From: Fan Du<fan.du@windriver.com> > Date: Wed, 16 Oct 2013 14:15:14 +0800 > >> Taking advantages of sadb_x_sec_ctx and xfrm_user_sec_ctx share the same >> structure arrangement, rework pfkey_sadb2xfrm_user_sec_ctx by casting >> sadb_x_sec_ctx into xfrm_user_sec_ctx with minor len fix. >> >> Then we can: >> -Avoid kmalloc/free memory for xfrm_user_sec_ctx, sadb_x_sec_ctx would be fine. >> -Fix missing return value check bug for pfkey_compile_policy when kmalloc fails >> >> Signed-off-by: Fan Du<fan.du@windriver.com> > > This isn't safe, one structure is packed and the other is not. Might be. No clue why "one structure is packed and the other is not" happens :( And why not pack the unpacked structure? or more generally does the packed structure in this case must be packed in this case?(I doubt this.) > Furthermore, unless there is some enormous gain (in this case there > is not) losing the type checking by casting two data structures like > this is undesirable. Comparing with the hot path optimization, yes this proposal doesn't bring great performance boosting. The aim of this patch is not the structure casting indeed but the avoiding kmalloc/memcpy for a PAGE_SIZE string("context" in SELINUX word) which maps into a ID for security checking against every AF_KEY operation.
diff --git a/net/key/af_key.c b/net/key/af_key.c index 9d58537..c7d304d 100644 --- a/net/key/af_key.c +++ b/net/key/af_key.c @@ -435,22 +435,9 @@ static inline int verify_sec_ctx_len(const void *p) static inline struct xfrm_user_sec_ctx *pfkey_sadb2xfrm_user_sec_ctx(const struct sadb_x_sec_ctx *sec_ctx) { - struct xfrm_user_sec_ctx *uctx = NULL; - int ctx_size = sec_ctx->sadb_x_ctx_len; - - uctx = kmalloc((sizeof(*uctx)+ctx_size), GFP_KERNEL); - - if (!uctx) - return NULL; + struct xfrm_user_sec_ctx *uctx = (struct xfrm_user_sec_ctx *)sec_ctx; uctx->len = pfkey_sec_ctx_len(sec_ctx); - uctx->exttype = sec_ctx->sadb_x_sec_exttype; - uctx->ctx_doi = sec_ctx->sadb_x_ctx_doi; - uctx->ctx_alg = sec_ctx->sadb_x_ctx_alg; - uctx->ctx_len = sec_ctx->sadb_x_ctx_len; - memcpy(uctx + 1, sec_ctx + 1, - uctx->ctx_len); - return uctx; } @@ -1125,12 +1112,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net, if (sec_ctx != NULL) { struct xfrm_user_sec_ctx *uctx = pfkey_sadb2xfrm_user_sec_ctx(sec_ctx); - if (!uctx) - goto out; - err = security_xfrm_state_alloc(x, uctx); - kfree(uctx); - if (err) goto out; } @@ -2225,14 +2207,7 @@ static int pfkey_spdadd(struct sock *sk, struct sk_buff *skb, const struct sadb_ if (sec_ctx != NULL) { struct xfrm_user_sec_ctx *uctx = pfkey_sadb2xfrm_user_sec_ctx(sec_ctx); - if (!uctx) { - err = -ENOBUFS; - goto out; - } - err = security_xfrm_policy_alloc(&xp->security, uctx); - kfree(uctx); - if (err) goto out; } @@ -2329,11 +2304,7 @@ static int pfkey_spddelete(struct sock *sk, struct sk_buff *skb, const struct sa if (sec_ctx != NULL) { struct xfrm_user_sec_ctx *uctx = pfkey_sadb2xfrm_user_sec_ctx(sec_ctx); - if (!uctx) - return -ENOMEM; - err = security_xfrm_policy_alloc(&pol_ctx, uctx); - kfree(uctx); if (err) return err; } @@ -3230,8 +3201,6 @@ static struct xfrm_policy *pfkey_compile_policy(struct sock *sk, int opt, goto out; uctx = pfkey_sadb2xfrm_user_sec_ctx(sec_ctx); *dir = security_xfrm_policy_alloc(&xp->security, uctx); - kfree(uctx); - if (*dir) goto out; }
Taking advantages of sadb_x_sec_ctx and xfrm_user_sec_ctx share the same structure arrangement, rework pfkey_sadb2xfrm_user_sec_ctx by casting sadb_x_sec_ctx into xfrm_user_sec_ctx with minor len fix. Then we can: -Avoid kmalloc/free memory for xfrm_user_sec_ctx, sadb_x_sec_ctx would be fine. -Fix missing return value check bug for pfkey_compile_policy when kmalloc fails Signed-off-by: Fan Du <fan.du@windriver.com> --- net/key/af_key.c | 33 +-------------------------------- 1 file changed, 1 insertion(+), 32 deletions(-)