Re: [PATCH net-next 1/2 v3] uevent: add alloc_uevent_skb() helper

[Christian Brauner]

On Fri, Apr 27, 2018 at 11:39:44AM -0500, Eric W. Biederman wrote:
> Christian Brauner  writes:
> 
> > This patch adds alloc_uevent_skb() in preparation for follow up patches.
> >
> > Signed-off-by: Christian Brauner 
> > ---
> >  lib/kobject_uevent.c | 39 ++-
> >  1 file changed, 26 insertions(+), 13 deletions(-)
> >
> > diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> > index 15ea216a67ce..c3cb110f663b 100644
> > --- a/lib/kobject_uevent.c
> > +++ b/lib/kobject_uevent.c
> > @@ -296,6 +296,31 @@ static void cleanup_uevent_env(struct subprocess_info 
> > *info)
> >  }
> >  #endif
> >  
> > +static struct sk_buff *alloc_uevent_skb(struct kobj_uevent_env *env,
> > +   const char *action_string,
> > +   const char *devpath)
> > +{
> > +   struct sk_buff *skb = NULL;
> > +   char *scratch;
> > +   size_t len;
> > +
> > +   /* allocate message with maximum possible size */
> > +   len = strlen(action_string) + strlen(devpath) + 2;
> > +   skb = alloc_skb(len + env->buflen, GFP_KERNEL);
> > +   if (!skb)
> > +   return NULL;
> > +
> > +   /* add header */
> > +   scratch = skb_put(skb, len);
> > +   sprintf(scratch, "%s@%s", action_string, devpath);
> > +
> > +   skb_put_data(skb, env->buf, env->buflen);
> > +
> > +   NETLINK_CB(skb).dst_group = 1;
> 
> nit:
>  We might want to explicitly set NETLINK_CB(skb).portid to 0 and
>  NETLINK_CB(skb).creds.uid to GLOBAL_ROOT_UID and
>  NETLINK_CB(skb).creds.gid to GLOBAL_ROOT_GID here
>  just to make it clear this is happening.
> 
>  It is not a problem because they __alloc_skb memsets to 0 the
>  fields of struct sk_buff that it does not initialize.  And these
>  are the zero values.
> 
>  Still it would be nice to be able to look at the code and quickly
>  see these are the values being set.

Don't really mind adding it. Ok, non-functional changes added to the new
version. But then let's set "portid" too:

parms = _CB(skb);
parms->creds.uid = GLOBAL_ROOT_UID;
parms->creds.gid = GLOBAL_ROOT_GID;
parms->dst_group = 1;
parms->portid = 0;

Christian

Re: [PATCH net-next 1/2 v3] uevent: add alloc_uevent_skb() helper

[Christian Brauner]

On Fri, Apr 27, 2018 at 11:39:44AM -0500, Eric W. Biederman wrote:
> Christian Brauner  writes:
> 
> > This patch adds alloc_uevent_skb() in preparation for follow up patches.
> >
> > Signed-off-by: Christian Brauner 
> > ---
> >  lib/kobject_uevent.c | 39 ++-
> >  1 file changed, 26 insertions(+), 13 deletions(-)
> >
> > diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> > index 15ea216a67ce..c3cb110f663b 100644
> > --- a/lib/kobject_uevent.c
> > +++ b/lib/kobject_uevent.c
> > @@ -296,6 +296,31 @@ static void cleanup_uevent_env(struct subprocess_info 
> > *info)
> >  }
> >  #endif
> >  
> > +static struct sk_buff *alloc_uevent_skb(struct kobj_uevent_env *env,
> > +   const char *action_string,
> > +   const char *devpath)
> > +{
> > +   struct sk_buff *skb = NULL;
> > +   char *scratch;
> > +   size_t len;
> > +
> > +   /* allocate message with maximum possible size */
> > +   len = strlen(action_string) + strlen(devpath) + 2;
> > +   skb = alloc_skb(len + env->buflen, GFP_KERNEL);
> > +   if (!skb)
> > +   return NULL;
> > +
> > +   /* add header */
> > +   scratch = skb_put(skb, len);
> > +   sprintf(scratch, "%s@%s", action_string, devpath);
> > +
> > +   skb_put_data(skb, env->buf, env->buflen);
> > +
> > +   NETLINK_CB(skb).dst_group = 1;
> 
> nit:
>  We might want to explicitly set NETLINK_CB(skb).portid to 0 and
>  NETLINK_CB(skb).creds.uid to GLOBAL_ROOT_UID and
>  NETLINK_CB(skb).creds.gid to GLOBAL_ROOT_GID here
>  just to make it clear this is happening.
> 
>  It is not a problem because they __alloc_skb memsets to 0 the
>  fields of struct sk_buff that it does not initialize.  And these
>  are the zero values.
> 
>  Still it would be nice to be able to look at the code and quickly
>  see these are the values being set.

Don't really mind adding it. Ok, non-functional changes added to the new
version. But then let's set "portid" too:

parms = _CB(skb);
parms->creds.uid = GLOBAL_ROOT_UID;
parms->creds.gid = GLOBAL_ROOT_GID;
parms->dst_group = 1;
parms->portid = 0;

Christian

Re: [PATCH net-next 1/2 v3] uevent: add alloc_uevent_skb() helper

[Eric W. Biederman]

Christian Brauner  writes:

> This patch adds alloc_uevent_skb() in preparation for follow up patches.
>
> Signed-off-by: Christian Brauner 
> ---
>  lib/kobject_uevent.c | 39 ++-
>  1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> index 15ea216a67ce..c3cb110f663b 100644
> --- a/lib/kobject_uevent.c
> +++ b/lib/kobject_uevent.c
> @@ -296,6 +296,31 @@ static void cleanup_uevent_env(struct subprocess_info 
> *info)
>  }
>  #endif
>  
> +static struct sk_buff *alloc_uevent_skb(struct kobj_uevent_env *env,
> + const char *action_string,
> + const char *devpath)
> +{
> + struct sk_buff *skb = NULL;
> + char *scratch;
> + size_t len;
> +
> + /* allocate message with maximum possible size */
> + len = strlen(action_string) + strlen(devpath) + 2;
> + skb = alloc_skb(len + env->buflen, GFP_KERNEL);
> + if (!skb)
> + return NULL;
> +
> + /* add header */
> + scratch = skb_put(skb, len);
> + sprintf(scratch, "%s@%s", action_string, devpath);
> +
> + skb_put_data(skb, env->buf, env->buflen);
> +
> + NETLINK_CB(skb).dst_group = 1;

nit:
 We might want to explicitly set NETLINK_CB(skb).portid to 0 and
 NETLINK_CB(skb).creds.uid to GLOBAL_ROOT_UID and
 NETLINK_CB(skb).creds.gid to GLOBAL_ROOT_GID here
 just to make it clear this is happening.

 It is not a problem because they __alloc_skb memsets to 0 the
 fields of struct sk_buff that it does not initialize.  And these
 are the zero values.

 Still it would be nice to be able to look at the code and quickly
 see these are the values being set.

Eric

Re: [PATCH net-next 1/2 v3] uevent: add alloc_uevent_skb() helper

[Eric W. Biederman]

Christian Brauner  writes:

> This patch adds alloc_uevent_skb() in preparation for follow up patches.
>
> Signed-off-by: Christian Brauner 
> ---
>  lib/kobject_uevent.c | 39 ++-
>  1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> index 15ea216a67ce..c3cb110f663b 100644
> --- a/lib/kobject_uevent.c
> +++ b/lib/kobject_uevent.c
> @@ -296,6 +296,31 @@ static void cleanup_uevent_env(struct subprocess_info 
> *info)
>  }
>  #endif
>  
> +static struct sk_buff *alloc_uevent_skb(struct kobj_uevent_env *env,
> + const char *action_string,
> + const char *devpath)
> +{
> + struct sk_buff *skb = NULL;
> + char *scratch;
> + size_t len;
> +
> + /* allocate message with maximum possible size */
> + len = strlen(action_string) + strlen(devpath) + 2;
> + skb = alloc_skb(len + env->buflen, GFP_KERNEL);
> + if (!skb)
> + return NULL;
> +
> + /* add header */
> + scratch = skb_put(skb, len);
> + sprintf(scratch, "%s@%s", action_string, devpath);
> +
> + skb_put_data(skb, env->buf, env->buflen);
> +
> + NETLINK_CB(skb).dst_group = 1;

nit:
 We might want to explicitly set NETLINK_CB(skb).portid to 0 and
 NETLINK_CB(skb).creds.uid to GLOBAL_ROOT_UID and
 NETLINK_CB(skb).creds.gid to GLOBAL_ROOT_GID here
 just to make it clear this is happening.

 It is not a problem because they __alloc_skb memsets to 0 the
 fields of struct sk_buff that it does not initialize.  And these
 are the zero values.

 Still it would be nice to be able to look at the code and quickly
 see these are the values being set.

Eric

[PATCH net-next 1/2 v3] uevent: add alloc_uevent_skb() helper

[Christian Brauner]

This patch adds alloc_uevent_skb() in preparation for follow up patches.

Signed-off-by: Christian Brauner 
---
 lib/kobject_uevent.c | 39 ++-
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 15ea216a67ce..c3cb110f663b 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -296,6 +296,31 @@ static void cleanup_uevent_env(struct subprocess_info 
*info)
 }
 #endif
 
+static struct sk_buff *alloc_uevent_skb(struct kobj_uevent_env *env,
+   const char *action_string,
+   const char *devpath)
+{
+   struct sk_buff *skb = NULL;
+   char *scratch;
+   size_t len;
+
+   /* allocate message with maximum possible size */
+   len = strlen(action_string) + strlen(devpath) + 2;
+   skb = alloc_skb(len + env->buflen, GFP_KERNEL);
+   if (!skb)
+   return NULL;
+
+   /* add header */
+   scratch = skb_put(skb, len);
+   sprintf(scratch, "%s@%s", action_string, devpath);
+
+   skb_put_data(skb, env->buf, env->buflen);
+
+   NETLINK_CB(skb).dst_group = 1;
+
+   return skb;
+}
+
 static int kobject_uevent_net_broadcast(struct kobject *kobj,
struct kobj_uevent_env *env,
const char *action_string,
@@ -314,22 +339,10 @@ static int kobject_uevent_net_broadcast(struct kobject 
*kobj,
continue;
 
if (!skb) {
-   /* allocate message with the maximum possible size */
-   size_t len = strlen(action_string) + strlen(devpath) + 
2;
-   char *scratch;
-
retval = -ENOMEM;
-   skb = alloc_skb(len + env->buflen, GFP_KERNEL);
+   skb = alloc_uevent_skb(env, action_string, devpath);
if (!skb)
continue;
-
-   /* add header */
-   scratch = skb_put(skb, len);
-   sprintf(scratch, "%s@%s", action_string, devpath);
-
-   skb_put_data(skb, env->buf, env->buflen);
-
-   NETLINK_CB(skb).dst_group = 1;
}
 
retval = netlink_broadcast_filtered(uevent_sock, skb_get(skb),
-- 
2.17.0

[PATCH net-next 1/2 v3] uevent: add alloc_uevent_skb() helper

[Christian Brauner]

This patch adds alloc_uevent_skb() in preparation for follow up patches.

Signed-off-by: Christian Brauner 
---
 lib/kobject_uevent.c | 39 ++-
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 15ea216a67ce..c3cb110f663b 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -296,6 +296,31 @@ static void cleanup_uevent_env(struct subprocess_info 
*info)
 }
 #endif
 
+static struct sk_buff *alloc_uevent_skb(struct kobj_uevent_env *env,
+   const char *action_string,
+   const char *devpath)
+{
+   struct sk_buff *skb = NULL;
+   char *scratch;
+   size_t len;
+
+   /* allocate message with maximum possible size */
+   len = strlen(action_string) + strlen(devpath) + 2;
+   skb = alloc_skb(len + env->buflen, GFP_KERNEL);
+   if (!skb)
+   return NULL;
+
+   /* add header */
+   scratch = skb_put(skb, len);
+   sprintf(scratch, "%s@%s", action_string, devpath);
+
+   skb_put_data(skb, env->buf, env->buflen);
+
+   NETLINK_CB(skb).dst_group = 1;
+
+   return skb;
+}
+
 static int kobject_uevent_net_broadcast(struct kobject *kobj,
struct kobj_uevent_env *env,
const char *action_string,
@@ -314,22 +339,10 @@ static int kobject_uevent_net_broadcast(struct kobject 
*kobj,
continue;
 
if (!skb) {
-   /* allocate message with the maximum possible size */
-   size_t len = strlen(action_string) + strlen(devpath) + 
2;
-   char *scratch;
-
retval = -ENOMEM;
-   skb = alloc_skb(len + env->buflen, GFP_KERNEL);
+   skb = alloc_uevent_skb(env, action_string, devpath);
if (!skb)
continue;
-
-   /* add header */
-   scratch = skb_put(skb, len);
-   sprintf(scratch, "%s@%s", action_string, devpath);
-
-   skb_put_data(skb, env->buf, env->buflen);
-
-   NETLINK_CB(skb).dst_group = 1;
}
 
retval = netlink_broadcast_filtered(uevent_sock, skb_get(skb),
-- 
2.17.0