Re: [patch 06/15] clocksource: Split out user string input

[John Stultz <john.stultz@linaro.org>]

On Thu, Apr 25, 2013 at 1:31 PM, Thomas Gleixner <tglx@linutronix.de> wrote:

> Split out the user string input for clocksource override. Preparatory
> patch for unbind.

This patch has a bug that causes the shell to hang when \n terminated 
clocksource strings are sent to the sysfs interface.

echo hpet > /sys/devices/system/clocksource/clocksource0/current_clocksource
<shell hang>


> +static size_t clocksource_get_uname(const char *buf, char *dst, size_t
> cnt)
> +{
> +       /* strings from sysfs write are not 0 terminated! */
> +       if (!cnt || cnt >= CS_NAME_LEN)
> +               return -EINVAL;
> +
> +       /* strip of \n: */
> +       if (buf[cnt-1] == '\n')
> +               cnt--;
> +       if (cnt > 0)
> +               memcpy(dst, buf, cnt);
> +       dst[cnt] = 0;
> +       return cnt;
> +}
> +
>  /**
>   * sysfs_override_clocksource - interface for manually overriding
> clocksource
>   * @dev:       unused
> @@ -847,22 +863,13 @@ static ssize_t sysfs_override_clocksourc
>                                           struct device_attribute *attr,
>                                           const char *buf, size_t count)
>  {
> -       size_t ret = count;
> -
> -       /* strings from sysfs write are not 0 terminated! */
> -       if (count >= sizeof(override_name))
> -               return -EINVAL;
> -
> -       /* strip of \n: */
> -       if (buf[count-1] == '\n')
> -               count--;
> +       size_t ret;
>
>         mutex_lock(&clocksource_mutex);
>
> -       if (count > 0)
> -               memcpy(override_name, buf, count);
> -       override_name[count] = 0;
> -       clocksource_select(false);
> +       ret = clocksource_get_uname(buf, override_name, count);
> +       if (ret >= 0)
> +               clocksource_select(false);
>
>         mutex_unlock(&clocksource_mutex);


The change above *looks* ok, but is subtlly different then what we had 
before. Note, in the old code, we saved count to ret, and never modified 
ret before returning it.

With the new code, we modify cnt, as we process the copied clocksource 
name, and return cnt, thus we don't return to the sysfs interface the 
same count value we were given, causing the write call to loop.

So you probably want something like the following patch to fix this.

thanks
-john



diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index e9c4f04..5f6c324 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -836,6 +836,8 @@ sysfs_show_current_clocksources(struct device *dev,

  static size_t clocksource_get_uname(const char *buf, char *dst, size_t 
cnt)
  {
+	size_t ret = cnt;
+
  	/* strings from sysfs write are not 0 terminated! */
  	if (!cnt || cnt >= CS_NAME_LEN)
  		return -EINVAL;
@@ -846,7 +848,7 @@ static size_t clocksource_get_uname(const char *buf, 
char *dst, size_t cnt)
  	if (cnt > 0)
  		memcpy(dst, buf, cnt);
  	dst[cnt] = 0;
-	return cnt;
+	return ret;
  }

  /**