[ 00/12] 2.6.32.59-longterm review

[ 00/12] 2.6.32.59-longterm review

[Willy Tarreau]

This is the start of the longterm review cycle for the 2.6.32.59 release.
All patches will be posted as a response to this one. If anyone has any
issue with these being applied, please let me know. If anyone is a
maintainer of the proper subsystem, and wants to add a Signed-off-by: line
to the patch, please respond with it.

Responses should be made within 72 hours. Anything received after that time
might be too late.

Please note that the whole -rc patch is not provided anymore, only individual
patches are provided so that their authors and subsystem maintainers can spot
issues. If this is a problem for you, please report it so that we try to find
a solution.

The diffstat is appended below.

 arch/ia64/Kconfig             |   17 -----------------
 arch/s390/Kconfig             |    3 +++
 arch/s390/kernel/setup.c      |    1 +
 block/bsg.c                   |    3 ++-
 drivers/net/usb/usbnet.c      |    2 ++
 drivers/watchdog/hpwdt.c      |    5 +++--
 fs/binfmt_elf.c               |    2 +-
 fs/cifs/dir.c                 |   20 ++++++++++++++++++--
 fs/ecryptfs/crypto.c          |   21 +++++++++++++++++++++
 fs/ecryptfs/ecryptfs_kernel.h |    2 ++
 fs/ecryptfs/file.c            |    3 ++-
 fs/ecryptfs/inode.c           |   18 +++---------------
 include/linux/backing-dev.h   |    1 +
 include/linux/regset.h        |   10 ++++++++--
 mm/backing-dev.c              |   15 ++++++++++-----
 mm/page-writeback.c           |    1 +
 net/mac80211/rate.c           |    2 +-
 17 files changed, 79 insertions(+), 47 deletions(-)

[ 01/12] compat: Re-add missing asm/compat.h include to fix compile breakage on s390

[Willy Tarreau]

2.6.32-longterm review patch.  If anyone has any objections, please let me know.

------------------

From: Heiko Carstens <heiko.carstens@de.ibm.com>

For kernels < 3.0 the backport of 048cd4e51d24ebf7f3552226d03c769d6ad91658
"compat: fix compile breakage on s390" will break compilation...

Re-add a single #include <asm/compat.h> in order to fix this.

This patch is _not_ necessary for upstream, only for stable kernels
which include the "build fix" mentioned above.

Reported-by: Jiri Slaby <jslaby@suse.cz>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 arch/s390/kernel/setup.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/s390/kernel/setup.c b/arch/s390/kernel/setup.c
index 0b2573a..358e545 100644
--- a/arch/s390/kernel/setup.c
+++ b/arch/s390/kernel/setup.c
@@ -57,6 +57,7 @@
 #include <asm/ptrace.h>
 #include <asm/sections.h>
 #include <asm/ebcdic.h>
+#include <asm/compat.h>
 #include <asm/kvm_virtio.h>
 
 long psw_kernel_bits	= (PSW_BASE_BITS | PSW_MASK_DAT | PSW_ASC_PRIMARY |
-- 
1.7.2.1.45.g54fbc

[ 02/12] Remove COMPAT_IA32 support

[Willy Tarreau]

2.6.32-longterm review patch.  If anyone has any objections, please let me know.

------------------

From: Ben Hutchings <ben@decadent.org.uk>

commit 32974ad4907cdde6c9de612cd1b2ee0568fb9409 upstream

This just changes Kconfig rather than touching all the other files the
original commit did.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 arch/ia64/Kconfig |   17 -----------------
 1 files changed, 0 insertions(+), 17 deletions(-)

diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index 1ee596c..20fc9c5 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -502,23 +502,6 @@ config ARCH_PROC_KCORE_TEXT
 	def_bool y
 	depends on PROC_KCORE
 
-config IA32_SUPPORT
-	bool "Support for Linux/x86 binaries"
-	help
-	  IA-64 processors can execute IA-32 (X86) instructions.  By
-	  saying Y here, the kernel will include IA-32 system call
-	  emulation support which makes it possible to transparently
-	  run IA-32 Linux binaries on an IA-64 Linux system.
-	  If in doubt, say Y.
-
-config COMPAT
-	bool
-	depends on IA32_SUPPORT
-	default y
-
-config COMPAT_FOR_U64_ALIGNMENT
-	def_bool COMPAT
-
 config IA64_MCA_RECOVERY
 	tristate "MCA recovery from errors other than TLB."
 
-- 
1.7.2.1.45.g54fbc

Re: [ 02/12] Remove COMPAT_IA32 support

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]

On Mon, 2012-03-12 at 01:20 +0100, Willy Tarreau wrote:
> 2.6.32-longterm review patch.  If anyone has any objections, please let me know.

The subject/first line should include '[IA64]', as in the original
commit.  It looks like this has been automatically stripped.

Ben.

> ------------------
> 
> From: Ben Hutchings <ben@decadent.org.uk>
> 
> commit 32974ad4907cdde6c9de612cd1b2ee0568fb9409 upstream
> 
> This just changes Kconfig rather than touching all the other files the
> original commit did.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
>  arch/ia64/Kconfig |   17 -----------------
>  1 files changed, 0 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
> index 1ee596c..20fc9c5 100644
> --- a/arch/ia64/Kconfig
> +++ b/arch/ia64/Kconfig
> @@ -502,23 +502,6 @@ config ARCH_PROC_KCORE_TEXT
>  	def_bool y
>  	depends on PROC_KCORE
>  
> -config IA32_SUPPORT
> -	bool "Support for Linux/x86 binaries"
> -	help
> -	  IA-64 processors can execute IA-32 (X86) instructions.  By
> -	  saying Y here, the kernel will include IA-32 system call
> -	  emulation support which makes it possible to transparently
> -	  run IA-32 Linux binaries on an IA-64 Linux system.
> -	  If in doubt, say Y.
> -
> -config COMPAT
> -	bool
> -	depends on IA32_SUPPORT
> -	default y
> -
> -config COMPAT_FOR_U64_ALIGNMENT
> -	def_bool COMPAT
> -
>  config IA64_MCA_RECOVERY
>  	tristate "MCA recovery from errors other than TLB."
>  

-- 
Ben Hutchings
Life would be so much easier if we could look at the source code.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [ 02/12] Remove COMPAT_IA32 support

[Greg KH]

On Mon, Mar 12, 2012 at 01:07:26AM +0000, Ben Hutchings wrote:
> On Mon, 2012-03-12 at 01:20 +0100, Willy Tarreau wrote:
> > 2.6.32-longterm review patch.  If anyone has any objections, please let me know.
> 
> The subject/first line should include '[IA64]', as in the original
> commit.  It looks like this has been automatically stripped.

Yeah, munging patches to and from quilt and git will cause that to
happen at times, it's quite common :(

greg k-h

Re: [ 02/12] Remove COMPAT_IA32 support

[Willy Tarreau]

On Sun, Mar 11, 2012 at 07:49:48PM -0700, Greg KH wrote:
> On Mon, Mar 12, 2012 at 01:07:26AM +0000, Ben Hutchings wrote:
> > On Mon, 2012-03-12 at 01:20 +0100, Willy Tarreau wrote:
> > > 2.6.32-longterm review patch.  If anyone has any objections, please let me know.
> > 
> > The subject/first line should include '[IA64]', as in the original
> > commit.  It looks like this has been automatically stripped.
> 
> Yeah, munging patches to and from quilt and git will cause that to
> happen at times, it's quite common :(

Indeed, and I've even changed my patch formats in haproxy to avoid brackets
due to this issue. The cause is that many patches are sent with a [PATCH]
prefix and that with Git, either you keep the subject line intact or you
remove everything that is between brackets. There's the -b option to only
remove remove tags looking like [PATCH], but my general experience with it
was not satisfying (I don't remind why). I have added some scripts to do
this at some points in the process but it's not 100% reliable as can be
seen here.

The "funny" thing here is that this comment lacking any IA64 tag annoyed
me a bit, and I was about to change its name then refrained from doing
so because I prefered not to keep the original message intact as I did
not notice the tag went off on my side :-/

Ben, I strongly suggest that you too switch your practise from "[IA64]"
to "IA64:" as can be seen on many kernel commits these days.

Thanks,
Willy

stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Jonathan Nieder]

Hi,

(adding git list and Thomas to cc)
Willy Tarreau wrote:
> On Sun, Mar 11, 2012 at 07:49:48PM -0700, Greg KH wrote:
>> On Mon, Mar 12, 2012 at 01:07:26AM +0000, Ben Hutchings wrote:

>>> The subject/first line should include '[IA64]', as in the original
>>> commit.  It looks like this has been automatically stripped.
>>
>> Yeah, munging patches to and from quilt and git will cause that to
>> happen at times, it's quite common :(
>
> Indeed, and I've even changed my patch formats in haproxy to avoid brackets
> due to this issue. The cause is that many patches are sent with a [PATCH]
> prefix and that with Git, either you keep the subject line intact or you
> remove everything that is between brackets. There's the -b option to only
> remove remove tags looking like [PATCH], but my general experience with it
> was not satisfying (I don't remind why).

Maybe the problem was as simple as "git am" not knowing about "-b".

Two relevant patches:

  f7e5ea17 (am: learn passing -b to mailinfo, 2012-01-16)
  ee2d1cb4 (mailinfo: with -b, keep space after [foo], 2012-01-16)

are in "master" and 1.7.10-rc0 and were not part of any earlier release.

Kudos to Thomas for writing them.

Jonathan

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Willy Tarreau]

Hi Jonathan,

On Mon, Mar 12, 2012 at 01:48:55AM -0500, Jonathan Nieder wrote:
> > Indeed, and I've even changed my patch formats in haproxy to avoid brackets
> > due to this issue. The cause is that many patches are sent with a [PATCH]
> > prefix and that with Git, either you keep the subject line intact or you
> > remove everything that is between brackets. There's the -b option to only
> > remove remove tags looking like [PATCH], but my general experience with it
> > was not satisfying (I don't remind why).
> 
> Maybe the problem was as simple as "git am" not knowing about "-b".

I think you're quite right.

> Two relevant patches:
> 
>   f7e5ea17 (am: learn passing -b to mailinfo, 2012-01-16)
>   ee2d1cb4 (mailinfo: with -b, keep space after [foo], 2012-01-16)
> 
> are in "master" and 1.7.10-rc0 and were not part of any earlier release.
>
> Kudos to Thomas for writing them.

Ah, thank you very much for this useful info, I'll update my version !

Cheers,
Willy

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Greg KH]

On Mon, Mar 12, 2012 at 09:58:20AM +0100, Willy Tarreau wrote:
> Hi Jonathan,
> 
> On Mon, Mar 12, 2012 at 01:48:55AM -0500, Jonathan Nieder wrote:
> > > Indeed, and I've even changed my patch formats in haproxy to avoid brackets
> > > due to this issue. The cause is that many patches are sent with a [PATCH]
> > > prefix and that with Git, either you keep the subject line intact or you
> > > remove everything that is between brackets. There's the -b option to only
> > > remove remove tags looking like [PATCH], but my general experience with it
> > > was not satisfying (I don't remind why).
> > 
> > Maybe the problem was as simple as "git am" not knowing about "-b".
> 
> I think you're quite right.
> 
> > Two relevant patches:
> > 
> >   f7e5ea17 (am: learn passing -b to mailinfo, 2012-01-16)
> >   ee2d1cb4 (mailinfo: with -b, keep space after [foo], 2012-01-16)
> > 
> > are in "master" and 1.7.10-rc0 and were not part of any earlier release.
> >
> > Kudos to Thomas for writing them.
> 
> Ah, thank you very much for this useful info, I'll update my version !

I don't see a -b option to 'git am' in the manpage, am I missing
something here?

greg k-h

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Willy Tarreau]

On Mon, Mar 12, 2012 at 08:20:04AM -0700, Greg KH wrote:
> > > Two relevant patches:
> > > 
> > >   f7e5ea17 (am: learn passing -b to mailinfo, 2012-01-16)
> > >   ee2d1cb4 (mailinfo: with -b, keep space after [foo], 2012-01-16)
> > > 
> > > are in "master" and 1.7.10-rc0 and were not part of any earlier release.
> > >
> > > Kudos to Thomas for writing them.
> > 
> > Ah, thank you very much for this useful info, I'll update my version !
> 
> I don't see a -b option to 'git am' in the manpage, am I missing
> something here?

It's in the master tree only right now, and the option is "--keep-non-patch"
(could have been shorter). Currently rebuilding to test it :-)

Willy

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Thomas Rast]

[+cc Junio because of backwards-compat issues]

Willy Tarreau <w@1wt.eu> writes:

> On Mon, Mar 12, 2012 at 08:20:04AM -0700, Greg KH wrote:
>> 
>> I don't see a -b option to 'git am' in the manpage, am I missing
>> something here?
>
> It's in the master tree only right now, and the option is "--keep-non-patch"
> (could have been shorter). Currently rebuilding to test it :-)

Exactly.

The problem with -b is that it's a backwards-compatibility shorthand for
--binary, which used to pass --allow-binary-replacement (or --binary) to
git-apply.  However, that option was obsoleted in 2b6eef9 (Make apply
--binary a no-op., 2006-09-06) and has been a no-op for over 5 years.
It has also not been documented since cb3a160 (git-am: ignore --binary
option, 2008-08-09).

So perhaps we can safely claim -b for --keep-non-patch, like so:

diff --git i/Documentation/git-am.txt w/Documentation/git-am.txt
index ee6cca2..9ec9313 100644
--- i/Documentation/git-am.txt
+++ w/Documentation/git-am.txt
@@ -40,6 +40,7 @@ OPTIONS
 --keep::
 	Pass `-k` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]).
 
+-b::
 --keep-non-patch::
 	Pass `-b` flag to 'git mailinfo' (see linkgit:git-mailinfo[1]).
 
diff --git i/git-am.sh w/git-am.sh
index 0bd290b..4b071de 100755
--- i/git-am.sh
+++ w/git-am.sh
@@ -9,13 +9,13 @@ git am [options] [(<mbox>|<Maildir>)...]
 git am [options] (--resolved | --skip | --abort)
 --
 i,interactive   run interactively
-b,binary*       (historical option -- no-op)
+binary*       (historical option -- no-op)
 3,3way          allow fall back on 3way merging if needed
 q,quiet         be quiet
 s,signoff       add a Signed-off-by line to the commit message
 u,utf8          recode into utf8 (default)
 k,keep          pass -k flag to git-mailinfo
-keep-non-patch  pass -b flag to git-mailinfo
+b,keep-non-patch pass -b flag to git-mailinfo
 keep-cr         pass --keep-cr flag to git-mailsplit for mbox format
 no-keep-cr      do not pass --keep-cr flag to git-mailsplit independent of am.keepcr
 c,scissors      strip everything before a scissors line
@@ -379,7 +379,7 @@ do
 	case "$1" in
 	-i|--interactive)
 		interactive=t ;;
-	-b|--binary)
+	--binary)
 		: ;;
 	-3|--3way)
 		threeway=t ;;
@@ -391,7 +391,7 @@ do
 		utf8= ;;
 	-k|--keep)
 		keep=t ;;
-	--keep-non-patch)
+	-b|--keep-non-patch)
 		keep=b ;;
 	-c|--scissors)
 		scissors=t ;;

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Willy Tarreau]

On Mon, Mar 12, 2012 at 05:41:49PM +0100, Thomas Rast wrote:
> [+cc Junio because of backwards-compat issues]
> 
> Willy Tarreau <w@1wt.eu> writes:
> 
> > On Mon, Mar 12, 2012 at 08:20:04AM -0700, Greg KH wrote:
> >> 
> >> I don't see a -b option to 'git am' in the manpage, am I missing
> >> something here?
> >
> > It's in the master tree only right now, and the option is "--keep-non-patch"
> > (could have been shorter). Currently rebuilding to test it :-)
> 
> Exactly.
> 
> The problem with -b is that it's a backwards-compatibility shorthand for
> --binary, which used to pass --allow-binary-replacement (or --binary) to
> git-apply.  However, that option was obsoleted in 2b6eef9 (Make apply
> --binary a no-op., 2006-09-06) and has been a no-op for over 5 years.
> It has also not been documented since cb3a160 (git-am: ignore --binary
> option, 2008-08-09).
> 
> So perhaps we can safely claim -b for --keep-non-patch, like so:

Yes I do think so, especially since 5 years ago, git commands were
called hyphenated like "git-am" instead of "git am". So I don't think
there's any risk in reusing the option.

Regards,
Willy

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Jonathan Nieder]

Thomas Rast wrote:

> The problem with -b is that it's a backwards-compatibility shorthand for
> --binary, which used to pass --allow-binary-replacement (or --binary) to
> git-apply.  However, that option was obsoleted in 2b6eef9 (Make apply
> --binary a no-op., 2006-09-06) and has been a no-op for over 5 years.
> It has also not been documented since cb3a160 (git-am: ignore --binary
> option, 2008-08-09).
>
> So perhaps we can safely claim -b for --keep-non-patch, like so:

Thanks.

It we want to be extra friendly to people who have been using
"format-patch --binary" with "am -b" in their scripts, we could have a
transitional period during which -b is treated as a usage error.

Luckily, a quick Google code search does not reveal any users for "am
-b", so I am not too worried and would not mind your patch that just
switches over right away, though.  After all, the failure modes are:

 - if my current script using "am -b" gets run using ancient git, it
   will accept binary patches and will strip out too many brackets
   in the subject line

 - if my ancient script using "am -b" gets run using current git, it
   will helpefully keep [IA64] brackets in the subject line

Neither seems terribly painful.

The manual would need to mention that this once meant --binary to
avoid confusion when that happens.

Hope that helps,
Jonathan

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Thomas Rast wrote:
>
>> The problem with -b is that it's a backwards-compatibility shorthand for
>> --binary, which used to pass --allow-binary-replacement (or --binary) to
>> git-apply.  However, that option was obsoleted in 2b6eef9 (Make apply
>> --binary a no-op., 2006-09-06) and has been a no-op for over 5 years.
>> It has also not been documented since cb3a160 (git-am: ignore --binary
>> option, 2008-08-09).
>>
>> So perhaps we can safely claim -b for --keep-non-patch, like so:

We can delete "git am -b" (as it was deprecated long time ago), wait
for a cycle or two, and then repurpose it.  I do not mind starting
the first step (delete, but do not say anything about repurposing)
before 1.7.10-rc1 happens.

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Willy Tarreau]

On Mon, Mar 12, 2012 at 11:04:23AM -0700, Junio C Hamano wrote:
> >> So perhaps we can safely claim -b for --keep-non-patch, like so:
> 
> We can delete "git am -b" (as it was deprecated long time ago), wait
> for a cycle or two, and then repurpose it.  I do not mind starting
> the first step (delete, but do not say anything about repurposing)
> before 1.7.10-rc1 happens.

>From my user experience and what I see on a number of coworkers, users
tend to make big jumps when they need a new feature, so in practice,
not offering the option in a version or two would probably not affect
most users, especially the ones still relying on the old behaviour. So
I don't see much benefit in waiting for repurposing the option.

Just my 2 cents,
Willy

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Jonathan Nieder]

Willy Tarreau wrote:

> From my user experience and what I see on a number of coworkers, users
> tend to make big jumps when they need a new feature, so in practice,
> not offering the option in a version or two would probably not affect
> most users, especially the ones still relying on the old behaviour. So
> I don't see much benefit in waiting for repurposing the option.

The benefit is that if it does turn out to be a regression, early
adopters will complain to us because their scripts have stopped
working and we get a chance to back out the change without having to
worry about others who have started to rely on the option.

Regards,
Jonathan

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Willy Tarreau]

On Mon, Mar 12, 2012 at 01:54:40PM -0500, Jonathan Nieder wrote:
> Willy Tarreau wrote:
> 
> > From my user experience and what I see on a number of coworkers, users
> > tend to make big jumps when they need a new feature, so in practice,
> > not offering the option in a version or two would probably not affect
> > most users, especially the ones still relying on the old behaviour. So
> > I don't see much benefit in waiting for repurposing the option.
> 
> The benefit is that if it does turn out to be a regression, early
> adopters will complain to us because their scripts have stopped
> working and we get a chance to back out the change without having to
> worry about others who have started to rely on the option.

Indeed you're right. I forgot the case of the old script relying
on the silently ignored parameter!

Regards,
Willy

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Thomas Rast wrote:
>>
>>> The problem with -b is that it's a backwards-compatibility shorthand for
>>> --binary, which used to pass --allow-binary-replacement (or --binary) to
>>> git-apply.  However, that option was obsoleted in 2b6eef9 (Make apply
>>> --binary a no-op., 2006-09-06) and has been a no-op for over 5 years.
>>> It has also not been documented since cb3a160 (git-am: ignore --binary
>>> option, 2008-08-09).
>>>
>>> So perhaps we can safely claim -b for --keep-non-patch, like so:
>
> We can delete "git am -b" (as it was deprecated long time ago), wait
> for a cycle or two, and then repurpose it.  I do not mind starting
> the first step (delete, but do not say anything about repurposing)
> before 1.7.10-rc1 happens.

Ok, but if I don't get to say anything about repurposing, can I at least
make it an error message instead of just spewing out the "invalid
option" help?

----- 8< -----
Subject: [PATCH] git-am: error out when seeing -b/--binary

The --binary option to git-apply has been a no-op since 2b6eef9 (Make
apply --binary a no-op., 2006-09-06) and was deprecated in cb3a160
(git-am: ignore --binary option, 2008-08-09).

We could remove it outright, but let's be nice to people who still
have scripts saying 'git am -b' (if they exist) and tell them the
reason for the sudden failure.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
 git-am.sh |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/git-am.sh b/git-am.sh
index 0bd290b..faae820 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -380,7 +380,9 @@ do
 	-i|--interactive)
 		interactive=t ;;
 	-b|--binary)
-		: ;;
+		echo >&2 "The -b/--binary option was deprecated in 1.6.0 and removed in 1.7.10."
+		die "Please adjust your scripts."
+		;;
 	-3|--3way)
 		threeway=t ;;
 	-s|--signoff)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] git-am: error out when seeing -b/--binary

[Jonathan Nieder]

Thomas Rast wrote:

> Ok, but if I don't get to say anything about repurposing, can I at least
> make it an error message instead of just spewing out the "invalid
> option" help?

I don't mind either way.

[...]
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -380,7 +380,9 @@ do
>  	-i|--interactive)
>  		interactive=t ;;
>  	-b|--binary)
> -		: ;;
> +		echo >&2 "The -b/--binary option was deprecated in 1.6.0 and removed in 1.7.10."
> +		die "Please adjust your scripts."
> +		;;

Hm, on second thought, if people are seeing this message, I would
prefer if they write to the mailing list so we can find out about it.
So I really would rather see this say

	--binary)
		: ;;

and have "-b" completely unrecognized, without any words in our
defense except for a note in the release notes mentioning the option's
removal and that it has been an unadvertised backward-compatibility
no-op since 1.6.0.

Jonathan

Re: [PATCH] git-am: error out when seeing -b/--binary

[Thomas Rast]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Hm, on second thought, if people are seeing this message, I would
> prefer if they write to the mailing list so we can find out about it.
> So I really would rather see this say
>
> 	--binary)
> 		: ;;
>
> and have "-b" completely unrecognized, without any words in our
> defense except for a note in the release notes mentioning the option's
> removal and that it has been an unadvertised backward-compatibility
> no-op since 1.6.0.

I'd hate doing that, mostly because other projects got me really angry
about similar issues, e.g., 71c020c (Disable asciidoc 8.4.1+ semantics
for `{plus}` and friends, 2009-07-25).

By the time I knew what the problem was, I figured posting anywhere was
useless since the change was already in the wild, and thus needed
working around on our end; and all I could possibly post was an angry
letter saying how unhappy I was about their work.

I didn't, and worked around it.  But that was after a lot of frustrated
investigation.  So I'd rather not do the same to our unlucky users.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] git-am: error out when seeing -b/--binary

[Jonathan Nieder]

Thomas Rast wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> Hm, on second thought, if people are seeing this message, I would
>> prefer if they write to the mailing list so we can find out about it.
>> So I really would rather see this say
>>
>> 	--binary)
>> 		: ;;
>>
>> and have "-b" completely unrecognized, without any words in our
>> defense except for a note in the release notes mentioning the option's
>> removal and that it has been an unadvertised backward-compatibility
>> no-op since 1.6.0.
>
> I'd hate doing that, mostly because other projects got me really angry
> about similar issues, e.g., 71c020c (Disable asciidoc 8.4.1+ semantics
> for `{plus}` and friends, 2009-07-25).

Oh, now that I think about it that way, you're definitely right.

So, how about something like this?

	--binary)
		: ;;
	-b)
		gettextln >&2 "The -b option (a no-op short for --binary) was removed in 1.7.10."
		die "$(gettext "Please adjust your scripts.")"
		;;

Mentioning deprecation in 1.6.0 in the message left me uneasy because
we never actually did anything to actively deprecate the option; it
just has not been needed since 1.4.3 and we stopped advertising it in
the manpage in 1.6.0.  So I don't like the implication of "this is all
right because we told you so" --- on the contrary, it is "in practice
nobody seems to be using this option and we hope nobody will notice
when we take it away".

Jonathan

Re: [PATCH] git-am: error out when seeing -b/--binary

[Thomas Rast]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 	--binary)
> 		: ;;
> 	-b)
> 		gettextln >&2 "The -b option (a no-op short for --binary) was removed in 1.7.10."
> 		die "$(gettext "Please adjust your scripts.")"
> 		;;
>
> Mentioning deprecation in 1.6.0 in the message left me uneasy because
> we never actually did anything to actively deprecate the option; it
> just has not been needed since 1.4.3 and we stopped advertising it in
> the manpage in 1.6.0.  So I don't like the implication of "this is all
> right because we told you so" --- on the contrary, it is "in practice
> nobody seems to be using this option and we hope nobody will notice
> when we take it away".

Hmm, I had an alternate patch ready in the morning, but Junio beat us to
it and applied the old one to master.

I don't really think it matters enough to apply *another*, so I'll leave
it at that.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] git-am: error out when seeing -b/--binary

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> 	--binary)
>> 		: ;;
>> 	-b)
>> 		gettextln >&2 "The -b option (a no-op short for --binary) was removed in 1.7.10."
>> 		die "$(gettext "Please adjust your scripts.")"
>> 		;;
>>
>> Mentioning deprecation in 1.6.0 in the message left me uneasy because
>> we never actually did anything to actively deprecate the option; it
>> just has not been needed since 1.4.3 and we stopped advertising it in
>> the manpage in 1.6.0.  So I don't like the implication of "this is all
>> right because we told you so" --- on the contrary, it is "in practice
>> nobody seems to be using this option and we hope nobody will notice
>> when we take it away".
>
> Hmm, I had an alternate patch ready in the morning, but Junio beat us to
> it and applied the old one to master.

I really don't think it is a good idea to avoid mentioning 1.6.0, at
which we *removed* description of the option in our manual pages and
from the "git am -h" help message. How much more active deprecation
would a user want?

To put it another way, think what your answer would be when somebody
sees the message and says "eh? all of a sudden it was removed?".
Wouldn't you tell him "At 1.6.0 we deprecated it and stopped
advertising it"?  Why not give that answer upfront?

Especially when you think "in practice nobody seems to be using
this" is true?

Re: [PATCH] git-am: error out when seeing -b/--binary

[Jonathan Nieder]

Junio C Hamano wrote:

> I really don't think it is a good idea to avoid mentioning 1.6.0, at
> which we *removed* description of the option in our manual pages and
> from the "git am -h" help message. How much more active deprecation
> would a user want?

A warning when the option is used or a mention in the release notes.

> To put it another way, think what your answer would be when somebody
> sees the message and says "eh? all of a sudden it was removed?".
> Wouldn't you tell him "At 1.6.0 we deprecated it and stopped
> advertising it"?  Why not give that answer upfront?

I would tell her "Since 1.4.3 it has been a compatibility no-op and
our documentation made that clear, and by now based on a search nobody
seems to be using it".

What happened in 1.6.0, then?  Well, before 1.6.0, the git-am(1)
manual said

 -b, --binary
	Pass --allow-binary-replacement flag to git-apply (see
	git-apply(1)).

and the git-apply(1) manual said

 --allow-binary-replacement, --binary
	Historically we did not allow binary patch applied
	without an explicit permission from the user, and this
	flag was the way to do so.  Currently we always allow binary
	patch application, so this is a no-op.

Afterwards, the entry in the git-am(1) manual was removed, saving the
reader a little time.

Jonathan

Re: [PATCH] git-am: error out when seeing -b/--binary

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> I really don't think it is a good idea to avoid mentioning 1.6.0, at
>> which we *removed* description of the option in our manual pages and
>> from the "git am -h" help message. How much more active deprecation
>> would a user want?
>
> A warning when the option is used or a mention in the release notes.

Ok, then we should probably instead do these two starting at 1.7.10 ("the
official deprecation date"), and then start erroring out on "-b" three
cycles after that.

Re: [PATCH] git-am: error out when seeing -b/--binary

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Hm, on second thought, if people are seeing this message, I would
> prefer if they write to the mailing list so we can find out about it.
> So I really would rather see this say
>
> 	--binary)
> 		: ;;
>
> and have "-b" completely unrecognized, without any words in our
> defense except for a note in the release notes mentioning the option's
> removal and that it has been an unadvertised backward-compatibility
> no-op since 1.6.0.

I do not mind keeping --binary working intact, but I think an
approach to say that "-b" no longer works and is finally removed
very firmly is a very sane one.  We would be getting a slightly
better feel of how stale the people's script could be with your
approach, but at the same time, we would be annoying far more people
who do not even know that there are some people passionately trying
to make the users' Git life better, or where these people are
hanging out.

I do not think removal of a no-op "-b" is something subject to
voting at this point, so the only thing you would get from the
better feel of the user universe is when we can repurpose the option
safely.

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Junio C Hamano]

Thomas Rast <trast@inf.ethz.ch> writes:

>> We can delete "git am -b" (as it was deprecated long time ago), wait
>> for a cycle or two, and then repurpose it.  I do not mind starting
>> the first step (delete, but do not say anything about repurposing)
>> before 1.7.10-rc1 happens.
>
> Ok, but if I don't get to say anything about repurposing, can I at least
> make it an error message instead of just spewing out the "invalid
> option" help?

Surely.  It is not "Am I at least allowed to say" at all; we really
_should_ make it clear why it is no longer supported.

What I meant was that "it will start meaning something different" is
not a relevant thing to see when somebody sees his old script that
uses "-b" breaks.

Let's apply your patch so that we do not have to wait one cycle too
long.

Thanks.

>
> ----- 8< -----
> Subject: [PATCH] git-am: error out when seeing -b/--binary
>
> The --binary option to git-apply has been a no-op since 2b6eef9 (Make
> apply --binary a no-op., 2006-09-06) and was deprecated in cb3a160
> (git-am: ignore --binary option, 2008-08-09).
>
> We could remove it outright, but let's be nice to people who still
> have scripts saying 'git am -b' (if they exist) and tell them the
> reason for the sudden failure.
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>  git-am.sh |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/git-am.sh b/git-am.sh
> index 0bd290b..faae820 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -380,7 +380,9 @@ do
>  	-i|--interactive)
>  		interactive=t ;;
>  	-b|--binary)
> -		: ;;
> +		echo >&2 "The -b/--binary option was deprecated in 1.6.0 and removed in 1.7.10."
> +		die "Please adjust your scripts."
> +		;;
>  	-3|--3way)
>  		threeway=t ;;
>  	-s|--signoff)

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Junio C Hamano]

Greg KH <greg@kroah.com> writes:

> I don't see a -b option to 'git am' in the manpage, am I missing
> something here?

As this is a recent enhancement, it is very much appreciated if you
try out 1.7.10-rc0 (or 'master').

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Willy Tarreau]

Hi Junio,

On Mon, Mar 12, 2012 at 09:40:48AM -0700, Junio C Hamano wrote:
> Greg KH <greg@kroah.com> writes:
> 
> > I don't see a -b option to 'git am' in the manpage, am I missing
> > something here?
> 
> As this is a recent enhancement, it is very much appreciated if you
> try out 1.7.10-rc0 (or 'master').

I've just backported it to 1.7.9.3 (I'm not keen on living on the bleeding
edge with my everyday tools), and it works nicely as expected.

Thanks!
Willy

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Junio C Hamano]

Willy Tarreau <w@1wt.eu> writes:

> Hi Junio,
>
> On Mon, Mar 12, 2012 at 09:40:48AM -0700, Junio C Hamano wrote:
>> Greg KH <greg@kroah.com> writes:
>> 
>> > I don't see a -b option to 'git am' in the manpage, am I missing
>> > something here?
>> 
>> As this is a recent enhancement, it is very much appreciated if you
>> try out 1.7.10-rc0 (or 'master').
>
> I've just backported it to 1.7.9.3 (I'm not keen on living on the bleeding
> edge with my everyday tools), and it works nicely as expected.

This topic will be backported in later versions of 1.7.9.x track,
but living on the maintenance track does not have much smaller
chance of breakage than living on the tip of 'master' these days,
unless you are using distro packaged version. The usual rule of
thumb if you are compiling from the source is that the tip of
'master' is more stable than any tagged version, including the
maintenance track.  See

http://thread.gmane.org/gmane.comp.version-control.git/189657/focus=190814

for details.

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Willy Tarreau]

On Mon, Mar 12, 2012 at 10:57:36AM -0700, Junio C Hamano wrote:
> > I've just backported it to 1.7.9.3 (I'm not keen on living on the bleeding
> > edge with my everyday tools), and it works nicely as expected.
> 
> This topic will be backported in later versions of 1.7.9.x track,
> but living on the maintenance track does not have much smaller
> chance of breakage than living on the tip of 'master' these days,
> unless you are using distro packaged version. The usual rule of
> thumb if you are compiling from the source is that the tip of
> 'master' is more stable than any tagged version, including the
> maintenance track.  See
> 
> http://thread.gmane.org/gmane.comp.version-control.git/189657/focus=190814
> 
> for details.

I know, but you see I was running on 1.7.2.3. Generally if one version
works for me, I don't upgrade it for a year or two. I've been hit a few
times in the past by some quite annoying bugs (as is to be expected from
any software in the development branch), and lost a lot of time on this.
Rest assured that I don't feel comfortable on 1.7.9 either, that's a big
jump for me but I know that most often it works quite well :-)

Willy

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Junio C Hamano]

Willy Tarreau <w@1wt.eu> writes:

> Rest assured that I don't feel comfortable on 1.7.9 either, that's a big
> jump for me but I know that most often it works quite well :-)

It being a big jump is not a problem I can solve, as it is up to you
to deliberately stay behind until you decide to make a big jump.

I can indirectly solve it by not backporting as many fixes to the
maintenance tracks, but I am too nice to be playing that nasty ;-).

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Greg KH]

On Mon, Mar 12, 2012 at 09:40:48AM -0700, Junio C Hamano wrote:
> Greg KH <greg@kroah.com> writes:
> 
> > I don't see a -b option to 'git am' in the manpage, am I missing
> > something here?
> 
> As this is a recent enhancement, it is very much appreciated if you
> try out 1.7.10-rc0 (or 'master').

Nice, I'll go do that now.

Does the flag propagate from 'git quiltimport'?  That's how I import
patches that would need this option most of the time.

thanks,

greg k-h

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Junio C Hamano]

Greg KH <greg@kroah.com> writes:

> Does the flag propagate from 'git quiltimport'?

I didn't even know the script was shipped as part of the main
Porcelain until I looked.  I do not think anybody is maintaining it;
the last update was from the 1.6.2 era.

A tested patch from quilt stakeholders is very much welcomed.

Thanks for bringing the script to my attention.

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Greg KH]

On Mon, Mar 12, 2012 at 11:01:55AM -0700, Junio C Hamano wrote:
> Greg KH <greg@kroah.com> writes:
> 
> > Does the flag propagate from 'git quiltimport'?
> 
> I didn't even know the script was shipped as part of the main
> Porcelain until I looked.  I do not think anybody is maintaining it;
> the last update was from the 1.6.2 era.

Ah, so my feeling that I'm the only one using it is true :)

> A tested patch from quilt stakeholders is very much welcomed.

I'll be glad to maintain this if you need me to.  I'll poke around with
it tomorrow to see what needs to be done here.

thanks,

greg k-h

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Junio C Hamano]

Greg KH <greg@kroah.com> writes:

> On Mon, Mar 12, 2012 at 11:01:55AM -0700, Junio C Hamano wrote:
>> Greg KH <greg@kroah.com> writes:
>> 
>> > Does the flag propagate from 'git quiltimport'?
>> 
>> I didn't even know the script was shipped as part of the main
>> Porcelain until I looked.  I do not think anybody is maintaining it;
>> the last update was from the 1.6.2 era.
>
> Ah, so my feeling that I'm the only one using it is true :)
>
>> A tested patch from quilt stakeholders is very much welcomed.
>
> I'll be glad to maintain this if you need me to.  I'll poke around with
> it tomorrow to see what needs to be done here.

Surely, and thanks.

Re: stripping [PATCH] without losing later tags from mailed patches (Re: [ 02/12] Remove COMPAT_IA32 support)

[Willy Tarreau]

On Mon, Mar 12, 2012 at 12:26:27PM -0700, Greg KH wrote:
> On Mon, Mar 12, 2012 at 11:01:55AM -0700, Junio C Hamano wrote:
> > Greg KH <greg@kroah.com> writes:
> > 
> > > Does the flag propagate from 'git quiltimport'?
> > 
> > I didn't even know the script was shipped as part of the main
> > Porcelain until I looked.  I do not think anybody is maintaining it;
> > the last update was from the 1.6.2 era.
> 
> Ah, so my feeling that I'm the only one using it is true :)

Well, we're two precisely :-)

> > A tested patch from quilt stakeholders is very much welcomed.
> 
> I'll be glad to maintain this if you need me to.  I'll poke around with
> it tomorrow to see what needs to be done here.

That would be great, this tool is really handy.

Cheers,
Willy

Re: [ 02/12] Remove COMPAT_IA32 support

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

On Mon, 2012-03-12 at 07:30 +0100, Willy Tarreau wrote:
> On Sun, Mar 11, 2012 at 07:49:48PM -0700, Greg KH wrote:
> > On Mon, Mar 12, 2012 at 01:07:26AM +0000, Ben Hutchings wrote:
> > > On Mon, 2012-03-12 at 01:20 +0100, Willy Tarreau wrote:
> > > > 2.6.32-longterm review patch.  If anyone has any objections, please let me know.
> > > 
> > > The subject/first line should include '[IA64]', as in the original
> > > commit.  It looks like this has been automatically stripped.
> > 
> > Yeah, munging patches to and from quilt and git will cause that to
> > happen at times, it's quite common :(
> 
> Indeed, and I've even changed my patch formats in haproxy to avoid brackets
> due to this issue. The cause is that many patches are sent with a [PATCH]
> prefix and that with Git, either you keep the subject line intact or you
> remove everything that is between brackets. There's the -b option to only
> remove remove tags looking like [PATCH], but my general experience with it
> was not satisfying (I don't remind why). I have added some scripts to do
> this at some points in the process but it's not 100% reliable as can be
> seen here.
> 
> The "funny" thing here is that this comment lacking any IA64 tag annoyed
> me a bit, and I was about to change its name then refrained from doing
> so because I prefered not to keep the original message intact as I did
> not notice the tag went off on my side :-/
> 
> Ben, I strongly suggest that you too switch your practise from "[IA64]"
> to "IA64:" as can be seen on many kernel commits these days.

That is my personal practice.  But I copied this subject from the
mainline commit.

Ben.

-- 
Ben Hutchings
Life would be so much easier if we could look at the source code.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [ 02/12] Remove COMPAT_IA32 support

[Arnd Bergmann]

On Monday 12 March 2012, Willy Tarreau wrote:
> 
> 2.6.32-longterm review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Ben Hutchings <ben@decadent.org.uk>
> 
> commit 32974ad4907cdde6c9de612cd1b2ee0568fb9409 upstream
> 
> This just changes Kconfig rather than touching all the other files the
> original commit did.
> 
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

I think it would be useful to copy the entire description of the
original patch here, which makes it clear why it's a good idea
to apply this patch to 2.6.32-longterm.

	Arnd

Re: [ 02/12] Remove COMPAT_IA32 support

[Willy Tarreau]

Hi Arnd,

On Mon, Mar 12, 2012 at 05:02:06PM +0000, Arnd Bergmann wrote:
> On Monday 12 March 2012, Willy Tarreau wrote:
> > 
> > 2.6.32-longterm review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Ben Hutchings <ben@decadent.org.uk>
> > 
> > commit 32974ad4907cdde6c9de612cd1b2ee0568fb9409 upstream
> > 
> > This just changes Kconfig rather than touching all the other files the
> > original commit did.
> > 
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> 
> I think it would be useful to copy the entire description of the
> original patch here, which makes it clear why it's a good idea
> to apply this patch to 2.6.32-longterm.

It's possible it was stripped by git-am as I don't have it in the
queued patch either. I agree with you that the more info we have
in stable commits, the best. I'll recheck the original mail.

Thanks,
Willy

Re: [ 02/12] Remove COMPAT_IA32 support

[Ben Hutchings]

On Mon, Mar 12, 2012 at 05:02:06PM +0000, Arnd Bergmann wrote:
> On Monday 12 March 2012, Willy Tarreau wrote:
> > 
> > 2.6.32-longterm review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Ben Hutchings <ben@decadent.org.uk>
> > 
> > commit 32974ad4907cdde6c9de612cd1b2ee0568fb9409 upstream
> > 
> > This just changes Kconfig rather than touching all the other files the
> > original commit did.
> > 
> > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> 
> I think it would be useful to copy the entire description of the
> original patch here, which makes it clear why it's a good idea
> to apply this patch to 2.6.32-longterm.
 
Agreed.  Willy, will you do this or should I send a new patch?

Ben.

-- 
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
                                                              - Albert Camus

Re: [ 02/12] Remove COMPAT_IA32 support

[Willy Tarreau]

On Mon, Mar 12, 2012 at 07:34:55PM +0000, Ben Hutchings wrote:
> On Mon, Mar 12, 2012 at 05:02:06PM +0000, Arnd Bergmann wrote:
> > On Monday 12 March 2012, Willy Tarreau wrote:
> > > 
> > > 2.6.32-longterm review patch.  If anyone has any objections, please let me know.
> > > 
> > > ------------------
> > > 
> > > From: Ben Hutchings <ben@decadent.org.uk>
> > > 
> > > commit 32974ad4907cdde6c9de612cd1b2ee0568fb9409 upstream
> > > 
> > > This just changes Kconfig rather than touching all the other files the
> > > original commit did.
> > > 
> > > Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> > 
> > I think it would be useful to copy the entire description of the
> > original patch here, which makes it clear why it's a good idea
> > to apply this patch to 2.6.32-longterm.
>  
> Agreed.  Willy, will you do this or should I send a new patch?

I'll do it, don't worry. I'm adding this by hand to the patch :

Original patch description :
--
    [IA64] Remove COMPAT_IA32 support
    
    This has been broken since May 2008 when Al Viro killed altroot support.
    Since nobody has complained, it would appear that there are no users of
    this code (A plausible theory since the main OSVs that support ia64 prefer
    to use the IA32-EL software emulation).
    
    Signed-off-by: Tony Luck <tony.luck@intel.com>
--

Thanks Ben,
Willy

[ 03/12] writeback: fixups for !dirty_writeback_centisecs

[Willy Tarreau]

2.6.32-longterm review patch.  If anyone has any objections, please let me know.

------------------

From: Jens Axboe <jens.axboe@oracle.com>

commit 6423104b6a1e6f0c18be60e8c33f02d263331d5e upstream.

Commit 69b62d01 fixed up most of the places where we would enter
busy schedule() spins when disabling the periodic background
writeback. This fixes up the sb timer so that it doesn't get
hammered on with the delay disabled, and ensures that it gets
rearmed if needed when /proc/sys/vm/dirty_writeback_centisecs
gets modified.

bdi_forker_task() also needs to check for !dirty_writeback_centisecs
and use schedule() appropriately, fix that up too.

Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
Tested-by: Xavier Roche <roche@httrack.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 include/linux/backing-dev.h |    1 +
 mm/backing-dev.c            |   15 ++++++++++-----
 mm/page-writeback.c         |    1 +
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index b449e73..61e43a6 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -105,6 +105,7 @@ void bdi_start_writeback(struct backing_dev_info *bdi, struct super_block *sb,
 				long nr_pages);
 int bdi_writeback_task(struct bdi_writeback *wb);
 int bdi_has_dirty_io(struct backing_dev_info *bdi);
+void bdi_arm_supers_timer(void);
 
 extern spinlock_t bdi_lock;
 extern struct list_head bdi_list;
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 67a33a5..d824401 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -41,7 +41,6 @@ static struct timer_list sync_supers_timer;
 
 static int bdi_sync_supers(void *);
 static void sync_supers_timer_fn(unsigned long);
-static void arm_supers_timer(void);
 
 static void bdi_add_default_flusher_task(struct backing_dev_info *bdi);
 
@@ -242,7 +241,7 @@ static int __init default_bdi_init(void)
 
 	init_timer(&sync_supers_timer);
 	setup_timer(&sync_supers_timer, sync_supers_timer_fn, 0);
-	arm_supers_timer();
+	bdi_arm_supers_timer();
 
 	err = bdi_init(&default_backing_dev_info);
 	if (!err)
@@ -364,10 +363,13 @@ static int bdi_sync_supers(void *unused)
 	return 0;
 }
 
-static void arm_supers_timer(void)
+void bdi_arm_supers_timer(void)
 {
 	unsigned long next;
 
+	if (!dirty_writeback_interval)
+		return;
+
 	next = msecs_to_jiffies(dirty_writeback_interval * 10) + jiffies;
 	mod_timer(&sync_supers_timer, round_jiffies_up(next));
 }
@@ -375,7 +377,7 @@ static void arm_supers_timer(void)
 static void sync_supers_timer_fn(unsigned long unused)
 {
 	wake_up_process(sync_supers_tsk);
-	arm_supers_timer();
+	bdi_arm_supers_timer();
 }
 
 static int bdi_forker_task(void *ptr)
@@ -418,7 +420,10 @@ static int bdi_forker_task(void *ptr)
 
 			spin_unlock_bh(&bdi_lock);
 			wait = msecs_to_jiffies(dirty_writeback_interval * 10);
-			schedule_timeout(wait);
+			if (wait)
+				schedule_timeout(wait);
+			else
+				schedule();
 			try_to_freeze();
 			continue;
 		}
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 2c5d792..52f71ae 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -694,6 +694,7 @@ int dirty_writeback_centisecs_handler(ctl_table *table, int write,
 	void __user *buffer, size_t *length, loff_t *ppos)
 {
 	proc_dointvec(table, write, buffer, length, ppos);
+	bdi_arm_supers_timer();
 	return 0;
 }
 
-- 
1.7.2.1.45.g54fbc

[ 04/12] bsg: fix sysfs link remove warning

[Willy Tarreau]

2.6.32-longterm review patch.  If anyone has any objections, please let me know.

------------------

From: Stanislaw Gruszka <sgruszka@redhat.com>

BugLink: http://bugs.launchpad.net/bugs/946928

We create "bsg" link if q->kobj.sd is not NULL, so remove it only
when the same condition is true.

Fixes:

WARNING: at fs/sysfs/inode.c:323 sysfs_hash_and_remove+0x2b/0x77()
sysfs: can not remove 'bsg', no directory
Call Trace:
  [<c0429683>] warn_slowpath_common+0x6a/0x7f
  [<c0537a68>] ? sysfs_hash_and_remove+0x2b/0x77
  [<c042970b>] warn_slowpath_fmt+0x2b/0x2f
  [<c0537a68>] sysfs_hash_and_remove+0x2b/0x77
  [<c053969a>] sysfs_remove_link+0x20/0x23
  [<c05d88f1>] bsg_unregister_queue+0x40/0x6d
  [<c0692263>] __scsi_remove_device+0x31/0x9d
  [<c069149f>] scsi_forget_host+0x41/0x52
  [<c0689fa9>] scsi_remove_host+0x71/0xe0
  [<f7de5945>] quiesce_and_remove_host+0x51/0x83 [usb_storage]
  [<f7de5a1e>] usb_stor_disconnect+0x18/0x22 [usb_storage]
  [<c06c29de>] usb_unbind_interface+0x4e/0x109
  [<c067a80f>] __device_release_driver+0x6b/0xa6
  [<c067a861>] device_release_driver+0x17/0x22
  [<c067a46a>] bus_remove_device+0xd6/0xe6
  [<c06785e2>] device_del+0xf2/0x137
  [<c06c101f>] usb_disable_device+0x94/0x1a0

Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>
(cherry picked from commit 37b40adf2d1b4a5e51323be73ccf8ddcf3f15dd3)

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 block/bsg.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/block/bsg.c b/block/bsg.c
index 7154a7a..e3e3241 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -977,7 +977,8 @@ void bsg_unregister_queue(struct request_queue *q)
 
 	mutex_lock(&bsg_mutex);
 	idr_remove(&bsg_minor_idr, bcd->minor);
-	sysfs_remove_link(&q->kobj, "bsg");
+	if (q->kobj.sd)
+		sysfs_remove_link(&q->kobj, "bsg");
 	device_unregister(bcd->class_dev);
 	bcd->class_dev = NULL;
 	kref_put(&bcd->ref, bsg_kref_release_function);
-- 
1.7.2.1.45.g54fbc

[ 05/12] eCryptfs: Handle failed metadata read in lookup

[Willy Tarreau]

2.6.32-longterm review patch.  If anyone has any objections, please let me know.

------------------

From: Tyler Hicks <tyhicks@linux.vnet.ibm.com>

When failing to read the lower file's crypto metadata during a lookup,
eCryptfs must continue on without throwing an error. For example, there
may be a plaintext file in the lower mount point that the user wants to
delete through the eCryptfs mount.

If an error is encountered while reading the metadata in lookup(), the
eCryptfs inode's size could be incorrect. We must be sure to reread the
plaintext inode size from the metadata when performing an open() or
setattr(). The metadata is already being read in those paths, so this
adds minimal performance overhead.

This patch introduces a flag which will track whether or not the
plaintext inode size has been read so that an incorrect i_size can be
fixed in the open() or setattr() paths.

BugLink: http://bugs.launchpad.net/bugs/509180
https://lists.ubuntu.com/archives/kernel-team/2012-March/019137.html

Cc: <stable@kernel.org>
Signed-off-by: Tyler Hicks <tyhicks@linux.vnet.ibm.com>

(backported from 3aeb86ea4cd15f728147a3bd5469a205ada8c767)
Cc: Tyler Hicks <tyler.hicks@canonical.com>
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 fs/ecryptfs/crypto.c          |   21 +++++++++++++++++++++
 fs/ecryptfs/ecryptfs_kernel.h |    2 ++
 fs/ecryptfs/file.c            |    3 ++-
 fs/ecryptfs/inode.c           |   18 +++---------------
 4 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
index 7a5f1ac..7e164bb 100644
--- a/fs/ecryptfs/crypto.c
+++ b/fs/ecryptfs/crypto.c
@@ -1455,6 +1455,25 @@ static void set_default_header_data(struct ecryptfs_crypt_stat *crypt_stat)
 		ECRYPTFS_MINIMUM_HEADER_EXTENT_SIZE;
 }
 
+void ecryptfs_i_size_init(const char *page_virt, struct inode *inode)
+{
+	struct ecryptfs_mount_crypt_stat *mount_crypt_stat;
+	struct ecryptfs_crypt_stat *crypt_stat;
+	u64 file_size;
+
+	crypt_stat = &ecryptfs_inode_to_private(inode)->crypt_stat;
+	mount_crypt_stat =
+		&ecryptfs_superblock_to_private(inode->i_sb)->mount_crypt_stat;
+	if (mount_crypt_stat->flags & ECRYPTFS_ENCRYPTED_VIEW_ENABLED) {
+		file_size = i_size_read(ecryptfs_inode_to_lower(inode));
+		if (crypt_stat->flags & ECRYPTFS_METADATA_IN_XATTR)
+			file_size += crypt_stat->num_header_bytes_at_front;
+	} else
+		file_size = get_unaligned_be64(page_virt);
+	i_size_write(inode, (loff_t)file_size);
+	crypt_stat->flags |= ECRYPTFS_I_SIZE_INITIALIZED;
+}
+
 /**
  * ecryptfs_read_headers_virt
  * @page_virt: The virtual address into which to read the headers
@@ -1485,6 +1504,8 @@ static int ecryptfs_read_headers_virt(char *page_virt,
 		rc = -EINVAL;
 		goto out;
 	}
+	if (!(crypt_stat->flags & ECRYPTFS_I_SIZE_INITIALIZED))
+		ecryptfs_i_size_init(page_virt, ecryptfs_dentry->d_inode);
 	offset += MAGIC_ECRYPTFS_MARKER_SIZE_BYTES;
 	rc = ecryptfs_process_flags(crypt_stat, (page_virt + offset),
 				    &bytes_read);
diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
index 542f625..9685315 100644
--- a/fs/ecryptfs/ecryptfs_kernel.h
+++ b/fs/ecryptfs/ecryptfs_kernel.h
@@ -270,6 +270,7 @@ struct ecryptfs_crypt_stat {
 #define ECRYPTFS_ENCFN_USE_MOUNT_FNEK 0x00001000
 #define ECRYPTFS_ENCFN_USE_FEK        0x00002000
 #define ECRYPTFS_UNLINK_SIGS	      0x00004000
+#define ECRYPTFS_I_SIZE_INITIALIZED   0x00008000
 	u32 flags;
 	unsigned int file_version;
 	size_t iv_bytes;
@@ -619,6 +620,7 @@ struct ecryptfs_open_req {
 int ecryptfs_interpose(struct dentry *hidden_dentry,
 		       struct dentry *this_dentry, struct super_block *sb,
 		       u32 flags);
+void ecryptfs_i_size_init(const char *page_virt, struct inode *inode);
 int ecryptfs_lookup_and_interpose_lower(struct dentry *ecryptfs_dentry,
 					struct dentry *lower_dentry,
 					struct inode *ecryptfs_dir_inode,
diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c
index 3015389..502b09f 100644
--- a/fs/ecryptfs/file.c
+++ b/fs/ecryptfs/file.c
@@ -237,7 +237,8 @@ static int ecryptfs_open(struct inode *inode, struct file *file)
 				goto out_free;
 			}
 			rc = 0;
-			crypt_stat->flags &= ~(ECRYPTFS_ENCRYPTED);
+			crypt_stat->flags &= ~(ECRYPTFS_I_SIZE_INITIALIZED
+						| ECRYPTFS_ENCRYPTED);
 			mutex_unlock(&crypt_stat->cs_mutex);
 			goto out;
 		}
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 4434e8f..90a6087 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -256,10 +256,8 @@ int ecryptfs_lookup_and_interpose_lower(struct dentry *ecryptfs_dentry,
 	struct dentry *lower_dir_dentry;
 	struct vfsmount *lower_mnt;
 	struct inode *lower_inode;
-	struct ecryptfs_mount_crypt_stat *mount_crypt_stat;
 	struct ecryptfs_crypt_stat *crypt_stat;
 	char *page_virt = NULL;
-	u64 file_size;
 	int rc = 0;
 
 	lower_dir_dentry = lower_dentry->d_parent;
@@ -334,18 +332,7 @@ int ecryptfs_lookup_and_interpose_lower(struct dentry *ecryptfs_dentry,
 		}
 		crypt_stat->flags |= ECRYPTFS_METADATA_IN_XATTR;
 	}
-	mount_crypt_stat = &ecryptfs_superblock_to_private(
-		ecryptfs_dentry->d_sb)->mount_crypt_stat;
-	if (mount_crypt_stat->flags & ECRYPTFS_ENCRYPTED_VIEW_ENABLED) {
-		if (crypt_stat->flags & ECRYPTFS_METADATA_IN_XATTR)
-			file_size = (crypt_stat->num_header_bytes_at_front
-				     + i_size_read(lower_dentry->d_inode));
-		else
-			file_size = i_size_read(lower_dentry->d_inode);
-	} else {
-		file_size = get_unaligned_be64(page_virt);
-	}
-	i_size_write(ecryptfs_dentry->d_inode, (loff_t)file_size);
+	ecryptfs_i_size_init(page_virt, ecryptfs_dentry->d_inode);
 out_free_kmem:
 	kmem_cache_free(ecryptfs_header_cache_2, page_virt);
 	goto out;
@@ -964,7 +951,8 @@ static int ecryptfs_setattr(struct dentry *dentry, struct iattr *ia)
 				goto out;
 			}
 			rc = 0;
-			crypt_stat->flags &= ~(ECRYPTFS_ENCRYPTED);
+			crypt_stat->flags &= ~(ECRYPTFS_I_SIZE_INITIALIZED
+						| ECRYPTFS_ENCRYPTED);
 		}
 	}
 	mutex_unlock(&crypt_stat->cs_mutex);
-- 
1.7.2.1.45.g54fbc

[ 06/12] [S390] KEYS: Enable the compat keyctl wrapper on s390x

[Willy Tarreau]

2.6.32-longterm review patch.  If anyone has any objections, please let me know.

------------------

From: David Howells <dhowells@redhat.com>

commit 1d057720609ed052a6371fe1d53300e5e6328e94 upstream.

Enable the compat keyctl wrapper on s390x so that 32-bit s390 userspace can
call the keyctl() syscall.

There's an s390x assembly wrapper that truncates all the register values to
32-bits and this then calls compat_sys_keyctl() - but the latter only exists if
CONFIG_KEYS_COMPAT is enabled, and the s390 Kconfig doesn't enable it.

Without this patch, 32-bit calls to the keyctl() syscall are given an ENOSYS
error:

	[root@devel4 ~]# keyctl show
	Session Keyring
	-3: key inaccessible (Function not implemented)

Signed-off-by: David Howells <dhowells@redhat.com>
Acked-by: dan@danny.cz
Cc: Carsten Otte <cotte@de.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: linux-s390@vger.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Signed-off-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
index d172758..6d99a5f 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -227,6 +227,9 @@ config COMPAT
 config SYSVIPC_COMPAT
 	def_bool y if COMPAT && SYSVIPC
 
+config KEYS_COMPAT
+	def_bool y if COMPAT && KEYS
+
 config AUDIT_ARCH
 	def_bool y
 

[ 07/12] cifs: fix dentry refcount leak when opening a FIFO on lookup

[Willy Tarreau]

2.6.32-longterm review patch.  If anyone has any objections, please let me know.

------------------

From: Jeff Layton <jlayton@redhat.com>

commit 5bccda0ebc7c0331b81ac47d39e4b920b198b2cd upstream.

The cifs code will attempt to open files on lookup under certain
circumstances. What happens though if we find that the file we opened
was actually a FIFO or other special file?

Currently, the open filehandle just ends up being leaked leading to
a dentry refcount mismatch and oops on umount. Fix this by having the
code close the filehandle on the server if it turns out not to be a
regular file. While we're at it, change this spaghetti if statement
into a switch too.

Cc: stable@vger.kernel.org
Reported-by: CAI Qian <caiqian@redhat.com>
Tested-by: CAI Qian <caiqian@redhat.com>
Reviewed-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
Signed-off-by: Steve French <smfrench@gmail.com>

diff --git a/fs/cifs/dir.c b/fs/cifs/dir.c
index 63a196b..bc7e244 100644
--- a/fs/cifs/dir.c
+++ b/fs/cifs/dir.c
@@ -584,10 +584,26 @@ cifs_lookup(struct inode *parent_dir_inode, struct dentry *direntry,
 			 * If either that or op not supported returned, follow
 			 * the normal lookup.
 			 */
-			if ((rc == 0) || (rc == -ENOENT))
+			switch (rc) {
+			case 0:
+				/*
+				 * The server may allow us to open things like
+				 * FIFOs, but the client isn't set up to deal
+				 * with that. If it's not a regular file, just
+				 * close it and proceed as if it were a normal
+				 * lookup.
+				 */
+				if (newInode && !S_ISREG(newInode->i_mode)) {
+					CIFSSMBClose(xid, pTcon, fileHandle);
+					break;
+				}
+			case -ENOENT:
 				posix_open = true;
-			else if ((rc == -EINVAL) || (rc != -EOPNOTSUPP))
+			case -EOPNOTSUPP:
+				break;
+			default:
 				pTcon->broken_posix_open = true;
+			}
 		}
 		if (!posix_open)
 			rc = cifs_get_inode_info_unix(&newInode, full_path,

[ 08/12] mac80211: zero initialize count field in ieee80211_tx_rate

[Willy Tarreau]

2.6.32-longterm review patch.  If anyone has any objections, please let me know.

------------------

From: Mohammed Shafi Shajakhan <mohammed@qca.qualcomm.com>

commit 8617b093d0031837a7be9b32bc674580cfb5f6b5 upstream.

rate control algorithms concludes the rate as invalid
with rate[i].idx < -1 , while they do also check for rate[i].count is
non-zero. it would be safer to zero initialize the 'count' field.
recently we had a ath9k rate control crash where the ath9k rate control
in ath_tx_status assumed to check only for rate[i].count being non-zero
in one instance and ended up in using invalid rate index for
'connection monitoring NULL func frames' which eventually lead to the crash.
thanks to Pavel Roskin for fixing it and finding the root cause.
https://bugzilla.redhat.com/show_bug.cgi?id=768639

Cc: stable@vger.kernel.org
Cc: Pavel Roskin <proski@gnu.org>
Signed-off-by: Mohammed Shafi Shajakhan <mohammed@qca.qualcomm.com>
Signed-off-by: John W. Linville <linville@tuxdriver.com>

diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
index ad64f4d..f9b8e81 100644
--- a/net/mac80211/rate.c
+++ b/net/mac80211/rate.c
@@ -344,7 +344,7 @@ void rate_control_get_rate(struct ieee80211_sub_if_data *sdata,
 	for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
 		info->control.rates[i].idx = -1;
 		info->control.rates[i].flags = 0;
-		info->control.rates[i].count = 1;
+		info->control.rates[i].count = 0;
 	}
 
 	if (sdata->local->hw.flags & IEEE80211_HW_HAS_RATE_CONTROL)

Re: [ 08/12] mac80211: zero initialize count field in ieee80211_tx_rate

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2345 bytes --]

On Mon, 2012-03-12 at 01:20 +0100, Willy Tarreau wrote:
> 2.6.32-longterm review patch.  If anyone has any objections, please let me know.
> 
> ------------------
> 
> From: Mohammed Shafi Shajakhan <mohammed@qca.qualcomm.com>
> 
> commit 8617b093d0031837a7be9b32bc674580cfb5f6b5 upstream.
> 
> rate control algorithms concludes the rate as invalid
> with rate[i].idx < -1 , while they do also check for rate[i].count is
> non-zero. it would be safer to zero initialize the 'count' field.
> recently we had a ath9k rate control crash where the ath9k rate control
> in ath_tx_status assumed to check only for rate[i].count being non-zero
> in one instance and ended up in using invalid rate index for
> 'connection monitoring NULL func frames' which eventually lead to the crash.
> thanks to Pavel Roskin for fixing it and finding the root cause.
> https://bugzilla.redhat.com/show_bug.cgi?id=768639

In 2.6.32, ath_tx_status() checks that rates[i].idx >= 0, so it properly
ignores these dummy entries.  Further, there is code further down the
rate_control_get_rate() function that sets .idx only and appears to
depend on the initialisation of .count = 1.

So I'm pretty sure this patch is wrong for 2.6.32; it could be
backported but I don't think the change is necessary anyway.

Ben.

> Cc: stable@vger.kernel.org
> Cc: Pavel Roskin <proski@gnu.org>
> Signed-off-by: Mohammed Shafi Shajakhan <mohammed@qca.qualcomm.com>
> Signed-off-by: John W. Linville <linville@tuxdriver.com>
> 
> diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
> index ad64f4d..f9b8e81 100644
> --- a/net/mac80211/rate.c
> +++ b/net/mac80211/rate.c
> @@ -344,7 +344,7 @@ void rate_control_get_rate(struct ieee80211_sub_if_data *sdata,
>  	for (i = 0; i < IEEE80211_TX_MAX_RATES; i++) {
>  		info->control.rates[i].idx = -1;
>  		info->control.rates[i].flags = 0;
> -		info->control.rates[i].count = 1;
> +		info->control.rates[i].count = 0;
>  	}
>  
>  	if (sdata->local->hw.flags & IEEE80211_HW_HAS_RATE_CONTROL)
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Ben Hutchings
Life would be so much easier if we could look at the source code.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [ 08/12] mac80211: zero initialize count field in ieee80211_tx_rate

[Mohammed Shafi Shajakhan]

Hi Ben,

On Monday 12 March 2012 07:27 AM, Ben Hutchings wrote:
> On Mon, 2012-03-12 at 01:20 +0100, Willy Tarreau wrote:
>> 2.6.32-longterm review patch.  If anyone has any objections, please let me know.
>>
>> ------------------
>>
>> From: Mohammed Shafi Shajakhan<mohammed@qca.qualcomm.com>
>>
>> commit 8617b093d0031837a7be9b32bc674580cfb5f6b5 upstream.
>>
>> rate control algorithms concludes the rate as invalid
>> with rate[i].idx<  -1 , while they do also check for rate[i].count is
>> non-zero. it would be safer to zero initialize the 'count' field.
>> recently we had a ath9k rate control crash where the ath9k rate control
>> in ath_tx_status assumed to check only for rate[i].count being non-zero
>> in one instance and ended up in using invalid rate index for
>> 'connection monitoring NULL func frames' which eventually lead to the crash.
>> thanks to Pavel Roskin for fixing it and finding the root cause.
>> https://bugzilla.redhat.com/show_bug.cgi?id=768639
>
> In 2.6.32, ath_tx_status() checks that rates[i].idx>= 0, so it properly
> ignores these dummy entries.  Further, there is code further down the
> rate_control_get_rate() function that sets .idx only and appears to
> depend on the initialisation of .count = 1.
>
> So I'm pretty sure this patch is wrong for 2.6.32; it could be
> backported but I don't think the change is necessary anyway.

true, but i think its better to initialize the count = 0 rather than 
count = 1, though the older version driver checks for rate[i].idx >= 0 
in ath_rc_tx_status. while the ath_tx_status has no such iteration in 
the older driver code.

>
> Ben.
>
>> Cc: stable@vger.kernel.org
>> Cc: Pavel Roskin<proski@gnu.org>
>> Signed-off-by: Mohammed Shafi Shajakhan<mohammed@qca.qualcomm.com>
>> Signed-off-by: John W. Linville<linville@tuxdriver.com>
>>
>> diff --git a/net/mac80211/rate.c b/net/mac80211/rate.c
>> index ad64f4d..f9b8e81 100644
>> --- a/net/mac80211/rate.c
>> +++ b/net/mac80211/rate.c
>> @@ -344,7 +344,7 @@ void rate_control_get_rate(struct ieee80211_sub_if_data *sdata,
>>   	for (i = 0; i<  IEEE80211_TX_MAX_RATES; i++) {
>>   		info->control.rates[i].idx = -1;
>>   		info->control.rates[i].flags = 0;
>> -		info->control.rates[i].count = 1;
>> +		info->control.rates[i].count = 0;
>>   	}
>>
>>   	if (sdata->local->hw.flags&  IEEE80211_HW_HAS_RATE_CONTROL)
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe stable" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>


-- 
thanks,
shafi

Re: [ 08/12] mac80211: zero initialize count field in ieee80211_tx_rate

[Willy Tarreau]

Hi Shafi,

On Mon, Mar 12, 2012 at 10:06:23AM +0530, Mohammed Shafi Shajakhan wrote:
> >So I'm pretty sure this patch is wrong for 2.6.32; it could be
> >backported but I don't think the change is necessary anyway.
> 
> true, but i think its better to initialize the count = 0 rather than 
> count = 1, though the older version driver checks for rate[i].idx >= 0 
> in ath_rc_tx_status. while the ath_tx_status has no such iteration in 
> the older driver code.

In practice, if the patch brings nothing and not even correctness, I'd
rather drop it than make us believe that some issue is fixed. However
if you think it does happen to fix a real issue in 2.6.32 (possibly
combined with some other missing patch), please tell me so and I will
happily undelete it.

Regards,
Willy

Re: [ 08/12] mac80211: zero initialize count field in ieee80211_tx_rate

[Mohammed Shafi Shajakhan]

Hi Willy,

> On Mon, Mar 12, 2012 at 10:06:23AM +0530, Mohammed Shafi Shajakhan wrote:
>>> So I'm pretty sure this patch is wrong for 2.6.32; it could be
>>> backported but I don't think the change is necessary anyway.
>>
>> true, but i think its better to initialize the count = 0 rather than
>> count = 1, though the older version driver checks for rate[i].idx>= 0
>> in ath_rc_tx_status. while the ath_tx_status has no such iteration in
>> the older driver code.
>
> In practice, if the patch brings nothing and not even correctness, I'd
> rather drop it than make us believe that some issue is fixed. However
> if you think it does happen to fix a real issue in 2.6.32 (possibly
> combined with some other missing patch), please tell me so and I will
> happily undelete it.
>

we can drop it. also as there was no driver code checking for 
rate[i].count in the 2.6.32 driver. i am also not sure this fixes 
something in 2.6.32 but the patch itself is correct.

Pavel fixed a rate control crash in ath9k because of invalid rate index 
(-1) access. we were wondering how is it possible the driver can be 
using a invalid rate index ?
we can simply rule out a rate is invalid checking for (rate[i].idx < 0 
&& rate[i].count != 0)

Pavel found that the driver previously checks "only" for rate[i].count 
as non-zero and this itself seems sufficient as we assumed the invalid 
rate indexes are initialized with  -1, 0 for rate[i].idx and 
rate[i].count respectively. later found that mac80211 rate[i].count 
initializes with '1' for the 'rate[i].count' field.

thus we generically fixed in mac80211(also as per doc) to avoid any such 
issues in any rate control, though we can blame the driver for not 
checking for invalid rate.idx while blindly believing on rate[i].count.

thank you!

-- 
thanks,
shafi

Re: [ 08/12] mac80211: zero initialize count field in ieee80211_tx_rate

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1415 bytes --]

On Mon, 2012-03-12 at 12:22 +0530, Mohammed Shafi Shajakhan wrote:
> Hi Willy,
> 
> > On Mon, Mar 12, 2012 at 10:06:23AM +0530, Mohammed Shafi Shajakhan wrote:
> >>> So I'm pretty sure this patch is wrong for 2.6.32; it could be
> >>> backported but I don't think the change is necessary anyway.
> >>
> >> true, but i think its better to initialize the count = 0 rather than
> >> count = 1, though the older version driver checks for rate[i].idx>= 0
> >> in ath_rc_tx_status. while the ath_tx_status has no such iteration in
> >> the older driver code.
> >
> > In practice, if the patch brings nothing and not even correctness, I'd
> > rather drop it than make us believe that some issue is fixed. However
> > if you think it does happen to fix a real issue in 2.6.32 (possibly
> > combined with some other missing patch), please tell me so and I will
> > happily undelete it.
> >
> 
> we can drop it. also as there was no driver code checking for 
> rate[i].count in the 2.6.32 driver. i am also not sure this fixes 
> something in 2.6.32 but the patch itself is correct.
[...]

Please read and answer the *whole* of my earlier message.  The later
code in the rate_control_get_rate() function in 2.6.32 does appear to
depend on .count = 1, and there may be code elsewhere that does so too.

Ben.

-- 
Ben Hutchings
Life would be so much easier if we could look at the source code.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [ 08/12] mac80211: zero initialize count field in ieee80211_tx_rate

[Mohammed Shafi Shajakhan]

Hi Ben,

On Monday 12 March 2012 08:53 PM, Ben Hutchings wrote:
> On Mon, 2012-03-12 at 12:22 +0530, Mohammed Shafi Shajakhan wrote:
>> Hi Willy,
>>
>>> On Mon, Mar 12, 2012 at 10:06:23AM +0530, Mohammed Shafi Shajakhan wrote:
>>>>> So I'm pretty sure this patch is wrong for 2.6.32; it could be
>>>>> backported but I don't think the change is necessary anyway.
>>>>
>>>> true, but i think its better to initialize the count = 0 rather than
>>>> count = 1, though the older version driver checks for rate[i].idx>= 0
>>>> in ath_rc_tx_status. while the ath_tx_status has no such iteration in
>>>> the older driver code.
>>>
>>> In practice, if the patch brings nothing and not even correctness, I'd
>>> rather drop it than make us believe that some issue is fixed. However
>>> if you think it does happen to fix a real issue in 2.6.32 (possibly
>>> combined with some other missing patch), please tell me so and I will
>>> happily undelete it.
>>>
>>
>> we can drop it. also as there was no driver code checking for
>> rate[i].count in the 2.6.32 driver. i am also not sure this fixes
>> something in 2.6.32 but the patch itself is correct.
> [...]
>
> Please read and answer the *whole* of my earlier message.  The later
> code in the rate_control_get_rate() function in 2.6.32 does appear to
> depend on .count = 1, and there may be code elsewhere that does so too.
>

are you referring to those code in tx.c ieee80211_tx_h_rate_control. 
sorry if i had again missed something.

-- 
thanks,
shafi

Re: [ 08/12] mac80211: zero initialize count field in ieee80211_tx_rate

[Mohammed Shafi Shajakhan]

On Monday 12 March 2012 09:25 PM, Mohammed Shafi Shajakhan wrote:
> Hi Ben,
>
> On Monday 12 March 2012 08:53 PM, Ben Hutchings wrote:
>> On Mon, 2012-03-12 at 12:22 +0530, Mohammed Shafi Shajakhan wrote:
>>> Hi Willy,
>>>
>>>> On Mon, Mar 12, 2012 at 10:06:23AM +0530, Mohammed Shafi Shajakhan
>>>> wrote:
>>>>>> So I'm pretty sure this patch is wrong for 2.6.32; it could be
>>>>>> backported but I don't think the change is necessary anyway.
>>>>>
>>>>> true, but i think its better to initialize the count = 0 rather than
>>>>> count = 1, though the older version driver checks for rate[i].idx>= 0
>>>>> in ath_rc_tx_status. while the ath_tx_status has no such iteration in
>>>>> the older driver code.
>>>>
>>>> In practice, if the patch brings nothing and not even correctness, I'd
>>>> rather drop it than make us believe that some issue is fixed. However
>>>> if you think it does happen to fix a real issue in 2.6.32 (possibly
>>>> combined with some other missing patch), please tell me so and I will
>>>> happily undelete it.
>>>>
>>>
>>> we can drop it. also as there was no driver code checking for
>>> rate[i].count in the 2.6.32 driver. i am also not sure this fixes
>>> something in 2.6.32 but the patch itself is correct.
>> [...]
>>
>> Please read and answer the *whole* of my earlier message. The later
>> code in the rate_control_get_rate() function in 2.6.32 does appear to
>> depend on .count = 1, and there may be code elsewhere that does so too.
>>
>
> are you referring to those code in tx.c ieee80211_tx_h_rate_control.
> sorry if i had again missed something.

if you are referring to that rate[0].count should be definitely 
non-zero. at least the first rate should be non-zero as the drivers rate 
control algorithm should properly fill it up, or mac80211's 
rate_control_send_low should fill it up by 1 for 'no ack frames'
(or)  max retries set by the driver for other frames(recently NULL func
  is added connection monitoring frame whose status will be needed).

-- 
thanks,
shafi

Re: [ 08/12] mac80211: zero initialize count field in ieee80211_tx_rate

[Willy Tarreau]

On Mon, Mar 12, 2012 at 01:57:52AM +0000, Ben Hutchings wrote:
> On Mon, 2012-03-12 at 01:20 +0100, Willy Tarreau wrote:
> > 2.6.32-longterm review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Mohammed Shafi Shajakhan <mohammed@qca.qualcomm.com>
> > 
> > commit 8617b093d0031837a7be9b32bc674580cfb5f6b5 upstream.
> > 
> > rate control algorithms concludes the rate as invalid
> > with rate[i].idx < -1 , while they do also check for rate[i].count is
> > non-zero. it would be safer to zero initialize the 'count' field.
> > recently we had a ath9k rate control crash where the ath9k rate control
> > in ath_tx_status assumed to check only for rate[i].count being non-zero
> > in one instance and ended up in using invalid rate index for
> > 'connection monitoring NULL func frames' which eventually lead to the crash.
> > thanks to Pavel Roskin for fixing it and finding the root cause.
> > https://bugzilla.redhat.com/show_bug.cgi?id=768639
> 
> In 2.6.32, ath_tx_status() checks that rates[i].idx >= 0, so it properly
> ignores these dummy entries.  Further, there is code further down the
> rate_control_get_rate() function that sets .idx only and appears to
> depend on the initialisation of .count = 1.
> 
> So I'm pretty sure this patch is wrong for 2.6.32; it could be
> backported but I don't think the change is necessary anyway.

Dropping it then, thanks Ben !

Willy

[ 09/12] net/usbnet: avoid recursive locking in usbnet_stop()

[Willy Tarreau]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2635 bytes --]

2.6.32-longterm review patch.  If anyone has any objections, please let me know.

------------------

From: Sebastian Siewior <bigeasy@linutronix.de>

commit 4231d47e6fe69f061f96c98c30eaf9fb4c14b96d upstream.

|kernel BUG at kernel/rtmutex.c:724!
|[<c029599c>] (rt_spin_lock_slowlock+0x108/0x2bc) from [<c01c2330>] (defer_bh+0x1c/0xb4)
|[<c01c2330>] (defer_bh+0x1c/0xb4) from [<c01c3afc>] (rx_complete+0x14c/0x194)
|[<c01c3afc>] (rx_complete+0x14c/0x194) from [<c01cac88>] (usb_hcd_giveback_urb+0xa0/0xf0)
|[<c01cac88>] (usb_hcd_giveback_urb+0xa0/0xf0) from [<c01e1ff4>] (musb_giveback+0x34/0x40)
|[<c01e1ff4>] (musb_giveback+0x34/0x40) from [<c01e2b1c>] (musb_advance_schedule+0xb4/0x1c0)
|[<c01e2b1c>] (musb_advance_schedule+0xb4/0x1c0) from [<c01e2ca8>] (musb_cleanup_urb.isra.9+0x80/0x8c)
|[<c01e2ca8>] (musb_cleanup_urb.isra.9+0x80/0x8c) from [<c01e2ed0>] (musb_urb_dequeue+0xec/0x108)
|[<c01e2ed0>] (musb_urb_dequeue+0xec/0x108) from [<c01cbb90>] (unlink1+0xbc/0xcc)
|[<c01cbb90>] (unlink1+0xbc/0xcc) from [<c01cc2ec>] (usb_hcd_unlink_urb+0x54/0xa8)
|[<c01cc2ec>] (usb_hcd_unlink_urb+0x54/0xa8) from [<c01c2a84>] (unlink_urbs.isra.17+0x2c/0x58)
|[<c01c2a84>] (unlink_urbs.isra.17+0x2c/0x58) from [<c01c2b44>] (usbnet_terminate_urbs+0x94/0x10c)
|[<c01c2b44>] (usbnet_terminate_urbs+0x94/0x10c) from [<c01c2d68>] (usbnet_stop+0x100/0x15c)
|[<c01c2d68>] (usbnet_stop+0x100/0x15c) from [<c020f718>] (__dev_close_many+0x94/0xc8)

defer_bh() takes the lock which is hold during unlink_urbs(). The safe
walk suggest that the skb will be removed from the list and this is done
by defer_bh() so it seems to be okay to drop the lock here.

Cc: stable@kernel.org
Reported-by: Aníbal Almeida Pinto <anibal.pinto@efacec.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Acked-by: Oliver Neukum <oliver@neukum.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index fae0fbd..81b96e3 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -589,6 +589,7 @@ static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)
 		entry = (struct skb_data *) skb->cb;
 		urb = entry->urb;
 
+		spin_unlock_irqrestore(&q->lock, flags);
 		// during some PM-driven resume scenarios,
 		// these (async) unlinks complete immediately
 		retval = usb_unlink_urb (urb);
@@ -596,6 +597,7 @@ static int unlink_urbs (struct usbnet *dev, struct sk_buff_head *q)
 			netdev_dbg(dev->net, "unlink urb err, %d\n", retval);
 		else
 			count++;
+		spin_lock_irqsave(&q->lock, flags);
 	}
 	spin_unlock_irqrestore (&q->lock, flags);
 	return count;

[ 10/12] regset: Prevent null pointer reference on readonly regsets

[Willy Tarreau]

2.6.32-longterm review patch.  If anyone has any objections, please let me know.

------------------

From: H. Peter Anvin <hpa@zytor.com>

commit c8e252586f8d5de906385d8cf6385fee289a825e upstream.

The regset common infrastructure assumed that regsets would always
have .get and .set methods, but not necessarily .active methods.
Unfortunately people have since written regsets without .set methods.

Rather than putting in stub functions everywhere, handle regsets with
null .get or .set methods explicitly.

Signed-off-by: H. Peter Anvin <hpa@zytor.com>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Roland McGrath <roland@hack.frob.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index bcb884e..07d096c 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1421,7 +1421,7 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
 	for (i = 1; i < view->n; ++i) {
 		const struct user_regset *regset = &view->regsets[i];
 		do_thread_regset_writeback(t->task, regset);
-		if (regset->core_note_type &&
+		if (regset->core_note_type && regset->get &&
 		    (!regset->active || regset->active(t->task, regset))) {
 			int ret;
 			size_t size = regset->n * regset->size;
diff --git a/include/linux/regset.h b/include/linux/regset.h
index 8abee65..5150fd1 100644
--- a/include/linux/regset.h
+++ b/include/linux/regset.h
@@ -335,6 +335,9 @@ static inline int copy_regset_to_user(struct task_struct *target,
 {
 	const struct user_regset *regset = &view->regsets[setno];
 
+	if (!regset->get)
+		return -EOPNOTSUPP;
+
 	if (!access_ok(VERIFY_WRITE, data, size))
 		return -EIO;
 
@@ -358,6 +361,9 @@ static inline int copy_regset_from_user(struct task_struct *target,
 {
 	const struct user_regset *regset = &view->regsets[setno];
 
+	if (!regset->set)
+		return -EOPNOTSUPP;
+
 	if (!access_ok(VERIFY_READ, data, size))
 		return -EIO;
 

[ 11/12] regset: Return -EFAULT, not -EIO, on host-side memory fault

[Willy Tarreau]

2.6.32-longterm review patch.  If anyone has any objections, please let me know.

------------------

From: H. Peter Anvin <hpa@zytor.com>

commit 5189fa19a4b2b4c3bec37c3a019d446148827717 upstream.

There is only one error code to return for a bad user-space buffer
pointer passed to a system call in the same address space as the
system call is executed, and that is EFAULT.  Furthermore, the
low-level access routines, which catch most of the faults, return
EFAULT already.

Signed-off-by: H. Peter Anvin <hpa@zytor.com>
Reviewed-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Roland McGrath <roland@hack.frob.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/include/linux/regset.h b/include/linux/regset.h
index 5150fd1..686f373 100644
--- a/include/linux/regset.h
+++ b/include/linux/regset.h
@@ -339,7 +339,7 @@ static inline int copy_regset_to_user(struct task_struct *target,
 		return -EOPNOTSUPP;
 
 	if (!access_ok(VERIFY_WRITE, data, size))
-		return -EIO;
+		return -EFAULT;
 
 	return regset->get(target, regset, offset, size, NULL, data);
 }
@@ -365,7 +365,7 @@ static inline int copy_regset_from_user(struct task_struct *target,
 		return -EOPNOTSUPP;
 
 	if (!access_ok(VERIFY_READ, data, size))
-		return -EIO;
+		return -EFAULT;
 
 	return regset->set(target, regset, offset, size, NULL, data);
 }

[ 12/12] watchdog: hpwdt: clean up set_memory_x call for 32 bit

[Willy Tarreau]

2.6.32-longterm review patch.  If anyone has any objections, please let me know.

------------------

From: Maxim Uvarov <maxim.uvarov@oracle.com>

commit 97d2a10d5804d585ab0b58efbd710948401b886a upstream.

1. address has to be page aligned.
2. set_memory_x uses page size argument, not size.
Bug causes with following commit:
	commit da28179b4e90dda56912ee825c7eaa62fc103797
	Author: Mingarelli, Thomas <Thomas.Mingarelli@hp.com>
	Date:   Mon Nov 7 10:59:00 2011 +0100

     watchdog: hpwdt: Changes to handle NX secure bit in 32bit path

    commit e67d668e147c3b4fec638c9e0ace04319f5ceccd upstream.

    This patch makes use of the set_memory_x() kernel API in order
    to make necessary BIOS calls to source NMIs.

Signed-off-by: Maxim Uvarov <maxim.uvarov@oracle.com>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
Cc: stable <stable@vger.kernel.org>

diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c
index 8464ea1..3c166d3 100644
--- a/drivers/watchdog/hpwdt.c
+++ b/drivers/watchdog/hpwdt.c
@@ -231,7 +231,7 @@ static int __devinit cru_detect(unsigned long map_entry,
 
 	cmn_regs.u1.reax = CRU_BIOS_SIGNATURE_VALUE;
 
-	set_memory_x((unsigned long)bios32_entrypoint, (2 * PAGE_SIZE));
+	set_memory_x((unsigned long)bios32_map, 2);
 	asminline_call(&cmn_regs, bios32_entrypoint);
 
 	if (cmn_regs.u1.ral != 0) {
@@ -250,7 +250,8 @@ static int __devinit cru_detect(unsigned long map_entry,
 			cru_rom_addr =
 				ioremap(cru_physical_address, cru_length);
 			if (cru_rom_addr) {
-				set_memory_x((unsigned long)cru_rom_addr, cru_length);
+				set_memory_x((unsigned long)cru_rom_addr & PAGE_MASK,
+					(cru_length + PAGE_SIZE - 1) >> PAGE_SHIFT);
 				retval = 0;
 			}
 		}