Difference between patch in XSA and patch checked in

Difference between patch in XSA and patch checked in

[George Dunlap]

Dear committers,

I spent way to many hours this afternoon trying to rebase the CentOS
patchqueue from 4.6.5 to 4.6.6.  In theory "git rebase" should detect
when duplicate patches are being merged over and forget the patch
automatically.

Unfortunately, this didn't work in a large number of cases because there
were minor changes between the patch published in the XSA and the patch
that ended up getting checked in.  One example of this is
xsa218-4.6/0003-gnttab-Avoid-potential-double-put-of-maptrack-entry.patch,
which in the advisory has:

+        gdprintk(XENLOG_WARNING, "Unstable handle %#x\n", op->handle);

But in the checked-in patch for stable-4.6 (c/s 819044abe4) has:

+        gdprintk(XENLOG_WARNING, "Unstable handle %u\n", op->handle);

So I spent some time writing a script that would automatically look for
a patch in the commit history with the same title.  This also didn't
work, because the *titles* often changed, in really minor ways.  One
example of this is xsa219-4.6.patch, which has the commit title:

x86/shadow: Hold references for the duration of emulated writes

But when checked in to stable-4.6, had this title (c/s 4d13019cb0):

x86/shadow: hold references for the duration of emulated writes

Another example is xsa222-1-4.6.patch, which had the following title :

xen/memory: Fix return value handing of guest_remove_page()

But was checked in to stable-4.6 with this this title (c/s d23eb82c8a):

memory: fix return value handing of guest_remove_page()

I know Lars has run into similar problems.  Having a computer be able to
easily tell whether a patch has been applied or not will save everyone a
lot of time in the long run, and is far more important than fixing a
printk string or fixing capitalization in a patch title.

Can I propose that committers should always check in the exact version
of the patch in the publicly-released advisory?  Preferably directly
from xsa.git, and with 'git am' (and not rebasing or modifying patches)?

"Bugs" in patch titles should be caught during pre-embargo review, and
left if they get missed; stylistic issues with the patch should be fixed
in follow-up patches.

Thanks,
 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Difference between patch in XSA and patch checked in

[Andrew Cooper]

On 23/08/17 17:35, George Dunlap wrote:
> Dear committers,
>
> I spent way to many hours this afternoon trying to rebase the CentOS
> patchqueue from 4.6.5 to 4.6.6.  In theory "git rebase" should detect
> when duplicate patches are being merged over and forget the patch
> automatically.
>
> Unfortunately, this didn't work in a large number of cases because there
> were minor changes between the patch published in the XSA and the patch
> that ended up getting checked in.  One example of this is
> xsa218-4.6/0003-gnttab-Avoid-potential-double-put-of-maptrack-entry.patch,
> which in the advisory has:
>
> +        gdprintk(XENLOG_WARNING, "Unstable handle %#x\n", op->handle);
>
> But in the checked-in patch for stable-4.6 (c/s 819044abe4) has:
>
> +        gdprintk(XENLOG_WARNING, "Unstable handle %u\n", op->handle);
>
> So I spent some time writing a script that would automatically look for
> a patch in the commit history with the same title.  This also didn't
> work, because the *titles* often changed, in really minor ways.  One
> example of this is xsa219-4.6.patch, which has the commit title:
>
> x86/shadow: Hold references for the duration of emulated writes
>
> But when checked in to stable-4.6, had this title (c/s 4d13019cb0):
>
> x86/shadow: hold references for the duration of emulated writes
>
> Another example is xsa222-1-4.6.patch, which had the following title :
>
> xen/memory: Fix return value handing of guest_remove_page()
>
> But was checked in to stable-4.6 with this this title (c/s d23eb82c8a):
>
> memory: fix return value handing of guest_remove_page()
>
> I know Lars has run into similar problems.  Having a computer be able to
> easily tell whether a patch has been applied or not will save everyone a
> lot of time in the long run, and is far more important than fixing a
> printk string or fixing capitalization in a patch title.
>
> Can I propose that committers should always check in the exact version
> of the patch in the publicly-released advisory?  Preferably directly
> from xsa.git, and with 'git am' (and not rebasing or modifying patches)?
>
> "Bugs" in patch titles should be caught during pre-embargo review, and
> left if they get missed; stylistic issues with the patch should be fixed
> in follow-up patches.

I agree.  I only ever use `git am` to apply XSAs to staging branches.

The one and only change I make is to add the CVE reference in to the
commit message if it only had an XSA reference in the published patch.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Difference between patch in XSA and patch checked in

[George Dunlap]

On 08/23/2017 05:39 PM, Andrew Cooper wrote:
> On 23/08/17 17:35, George Dunlap wrote:
>> Dear committers,
>>
>> I spent way to many hours this afternoon trying to rebase the CentOS
>> patchqueue from 4.6.5 to 4.6.6.  In theory "git rebase" should detect
>> when duplicate patches are being merged over and forget the patch
>> automatically.
>>
>> Unfortunately, this didn't work in a large number of cases because there
>> were minor changes between the patch published in the XSA and the patch
>> that ended up getting checked in.  One example of this is
>> xsa218-4.6/0003-gnttab-Avoid-potential-double-put-of-maptrack-entry.patch,
>> which in the advisory has:
>>
>> +        gdprintk(XENLOG_WARNING, "Unstable handle %#x\n", op->handle);
>>
>> But in the checked-in patch for stable-4.6 (c/s 819044abe4) has:
>>
>> +        gdprintk(XENLOG_WARNING, "Unstable handle %u\n", op->handle);
>>
>> So I spent some time writing a script that would automatically look for
>> a patch in the commit history with the same title.  This also didn't
>> work, because the *titles* often changed, in really minor ways.  One
>> example of this is xsa219-4.6.patch, which has the commit title:
>>
>> x86/shadow: Hold references for the duration of emulated writes
>>
>> But when checked in to stable-4.6, had this title (c/s 4d13019cb0):
>>
>> x86/shadow: hold references for the duration of emulated writes
>>
>> Another example is xsa222-1-4.6.patch, which had the following title :
>>
>> xen/memory: Fix return value handing of guest_remove_page()
>>
>> But was checked in to stable-4.6 with this this title (c/s d23eb82c8a):
>>
>> memory: fix return value handing of guest_remove_page()
>>
>> I know Lars has run into similar problems.  Having a computer be able to
>> easily tell whether a patch has been applied or not will save everyone a
>> lot of time in the long run, and is far more important than fixing a
>> printk string or fixing capitalization in a patch title.
>>
>> Can I propose that committers should always check in the exact version
>> of the patch in the publicly-released advisory?  Preferably directly
>> from xsa.git, and with 'git am' (and not rebasing or modifying patches)?
>>
>> "Bugs" in patch titles should be caught during pre-embargo review, and
>> left if they get missed; stylistic issues with the patch should be fixed
>> in follow-up patches.
> 
> I agree.  I only ever use `git am` to apply XSAs to staging branches.
> 
> The one and only change I make is to add the CVE reference in to the
> commit message if it only had an XSA reference in the published patch.

Yes, I think adding tags like CVE numbers, or Reported-by's, is necessary.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Difference between patch in XSA and patch checked in

[Jan Beulich]

>>> On 23.08.17 at 18:35, <george.dunlap@citrix.com> wrote:
> Can I propose that committers should always check in the exact version
> of the patch in the publicly-released advisory?  Preferably directly
> from xsa.git, and with 'git am' (and not rebasing or modifying patches)?

As the presumably primary guilty one here, I'll try to remember to
not make such changes going forward. It is largely the adding of
CVE numbers and tags to the patch which has turned out easier to
do in a private copy of the patches (so they're ready to be applied
without having to wait for / pull updates to xsa.git, the more that
in less simple cases - which iirc XSA-218 was an example of - the
automatic propagation of tags into the patches at public disclosure
time doesn't always work [reliably]).

That's in particular how the format string differences have crept in
that have caused you grief, as the way the diff-ing works is
apparently quite different between the various possible tools to
use. I do compare patches in such cases in order to make sure I
don't commit any stale version, but the patch representation was
so different that I apparently didn't notice the mixup in format
strings.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Difference between patch in XSA and patch checked in

[George Dunlap]

On 08/24/2017 08:29 AM, Jan Beulich wrote:
>>>> On 23.08.17 at 18:35, <george.dunlap@citrix.com> wrote:
>> Can I propose that committers should always check in the exact version
>> of the patch in the publicly-released advisory?  Preferably directly
>> from xsa.git, and with 'git am' (and not rebasing or modifying patches)?
> 
> As the presumably primary guilty one here, I'll try to remember to
> not make such changes going forward. 

Just to be clear, I wasn't trying to call anybody out; I was just trying
to share my experience. :-)  Thanks for making the effort.

> It is largely the adding of
> CVE numbers and tags to the patch which has turned out easier to
> do in a private copy of the patches (so they're ready to be applied
> without having to wait for / pull updates to xsa.git, the more that
> in less simple cases - which iirc XSA-218 was an example of - the
> automatic propagation of tags into the patches at public disclosure
> time doesn't always work [reliably]).

Is there a "timeliness" issue for checking patches into the tree?

> That's in particular how the format string differences have crept in
> that have caused you grief, as the way the diff-ing works is
> apparently quite different between the various possible tools to
> use. I do compare patches in such cases in order to make sure I
> don't commit any stale version, but the patch representation was
> so different that I apparently didn't notice the mixup in format
> strings.

It sounds like maybe we could use a tool that verified that the state of
the tree after applying patch A and the state of the tree after applying
patch B are identical.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Difference between patch in XSA and patch checked in

[Jan Beulich]

>>> On 24.08.17 at 12:17, <george.dunlap@citrix.com> wrote:
> On 08/24/2017 08:29 AM, Jan Beulich wrote:
>> It is largely the adding of
>> CVE numbers and tags to the patch which has turned out easier to
>> do in a private copy of the patches (so they're ready to be applied
>> without having to wait for / pull updates to xsa.git, the more that
>> in less simple cases - which iirc XSA-218 was an example of - the
>> automatic propagation of tags into the patches at public disclosure
>> time doesn't always work [reliably]).
> 
> Is there a "timeliness" issue for checking patches into the tree?

Well, I've been striving to commit patches pretty quickly after
advisories went public.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Difference between patch in XSA and patch checked in

[George Dunlap]

On 08/24/2017 11:20 AM, Jan Beulich wrote:
>>>> On 24.08.17 at 12:17, <george.dunlap@citrix.com> wrote:
>> On 08/24/2017 08:29 AM, Jan Beulich wrote:
>>> It is largely the adding of
>>> CVE numbers and tags to the patch which has turned out easier to
>>> do in a private copy of the patches (so they're ready to be applied
>>> without having to wait for / pull updates to xsa.git, the more that
>>> in less simple cases - which iirc XSA-218 was an example of - the
>>> automatic propagation of tags into the patches at public disclosure
>>> time doesn't always work [reliably]).
>>
>> Is there a "timeliness" issue for checking patches into the tree?
> 
> Well, I've been striving to commit patches pretty quickly after
> advisories went public.

I understood that, but I didn't understand why.

I normally try to kick off CentOS builds of updated Xen packages at the
instant the embargo lifts, because 1) the CBS is public (and thus the
build can't start until the embargo lifts) and 2) the sooner the build
finishes, the sooner the users can test & reboot.

But for checking fixes into the public trees, I don't immediately see
why checking them in at UTC 12:01 is better than checking them in at UTC
1300 (or UTC 1700).  Nicer to have things out of the way, to be sure,
but not (as far as I can see at the moment) worth the extra effort and
risk of creating private patches to achieve.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel