| Message ID | 20210810173000.050147269@linuxfoundation.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Wed, Aug 11, 2021 at 09:28:43AM +0200, Pavel Machek wrote: > Hi! > > > From: Jonathan Gray <jsg@jsg.id.au> > > > > The backport of c9d9fdbc108af8915d3f497bbdf3898bf8f321b8 to 5.10 in > > 6976f3cf34a1a8b791c048bbaa411ebfe48666b1 removed more than it should > > have leading to 'batch' being used uninitialised. The 5.13 backport and > > the mainline commit did not remove the portion this patch adds back. > > This patch has no upstream equivalent, right? > > Which is okay -- it explains it in plain english, but it shows that > scripts should not simply search for anything that looks like SHA and > treat it as upsteam commit it. Sounds like you have a broken script if you do it that way. good luck! greg k-h
On Wed 2021-08-11 09:46:12, Greg Kroah-Hartman wrote: > On Wed, Aug 11, 2021 at 09:28:43AM +0200, Pavel Machek wrote: > > Hi! > > > > > From: Jonathan Gray <jsg@jsg.id.au> > > > > > > The backport of c9d9fdbc108af8915d3f497bbdf3898bf8f321b8 to 5.10 in > > > 6976f3cf34a1a8b791c048bbaa411ebfe48666b1 removed more than it should > > > have leading to 'batch' being used uninitialised. The 5.13 backport and > > > the mainline commit did not remove the portion this patch adds back. > > > > This patch has no upstream equivalent, right? > > > > Which is okay -- it explains it in plain english, but it shows that > > scripts should not simply search for anything that looks like SHA and > > treat it as upsteam commit it. > > Sounds like you have a broken script if you do it that way. That is what you told me to do! https://lore.kernel.org/stable/YQEvUay+1Rzp04SO@kroah.com/ I would happily adapt my script, but there's no good/documented/working way to determine upstream commit given -stable commit. If we could agree on Commit: (SHA) in the beggining of body, that would be great. Upstream: (SHA) in sign-off area would be even better. Best regards, Pavel
Hi! > > > > > From: Jonathan Gray <jsg@jsg.id.au> > > > > > > > > > > The backport of c9d9fdbc108af8915d3f497bbdf3898bf8f321b8 to 5.10 in > > > > > 6976f3cf34a1a8b791c048bbaa411ebfe48666b1 removed more than it should > > > > > have leading to 'batch' being used uninitialised. The 5.13 backport and > > > > > the mainline commit did not remove the portion this patch adds back. > > > > > > > > This patch has no upstream equivalent, right? > > > > > > > > Which is okay -- it explains it in plain english, but it shows that > > > > scripts should not simply search for anything that looks like SHA and > > > > treat it as upsteam commit it. > > > > > > Sounds like you have a broken script if you do it that way. > > > > That is what you told me to do! > > > > https://lore.kernel.org/stable/YQEvUay+1Rzp04SO@kroah.com/ > > Yes, which is fine for matching sha1 values. I'd really like reliable / automated way to tell upstream commit from given -stable commit. > > I would happily adapt my script, but there's no > > good/documented/working way to determine upstream commit given -stable > > commit. > > > > If we could agree on > > > > Commit: (SHA) > > > > in the beggining of body, that would be great. > > > > Upstream: (SHA) > > > > in sign-off area would be even better. > > What exactly are you trying to do when you find a sha1? For some reason > my scripts work just fine with a semi-free-form way that we currently > have been doing this for the past 17+ years. What are you attempting to > do that requires such a fixed format? Is there any problem having a fixed format? You are producing -stable kernels, so you are not the one needing such functionality. Anyway... We do reviews here, and we review patches on multiple branches (4.4, 4.19, 5.10). So I'm using scripts to group backports of the same patch together, for easier review and to flag patches that do not have upstream equivalent for extra review. (Example of the review file below) There are other uses. When we were creating 5.10-cip branch, we used automatic scripts to verify that all patches from 4.19-cip are included in 5.10-cip. Determining mainline patch for given commit was essential for that. Other use was actually suggested by you: you jokingly wanted to replace CVE-XXX with mainline commit IDs. But that needs reliable way to determine upstream commit from stable commit to work nicely. (And yes, we are actually trying to maintain the mapping, see for example https://gitlab.com/cip-project/cip-kernel/cip-kernel-sec/-/blob/master/issues/CVE-2016-10147.yml ) Best regards, Pavel a |150198841 135cbd o: 5.10| spi: imx: mx51-ecspi: Reinstate low-speed CONFIGREG delay a |8916a8606 53ca18 o: 5.10| spi: imx: mx51-ecspi: Fix low-speed CONFIGREG delay calculation v |2c32af963 5c0424 o: 5.10| scsi: sr: Return correct event when media event code is 3 v |671402b0e 5c0424 o: 4.19| scsi: sr: Return correct event when media event code is 3 a |a78c94304 5c0424 o: 4.4| scsi: sr: Return correct event when media event code is 3 v |9acc1f082 c592b4 o: 5.10| media: videobuf2-core: dequeue if start_streaming fails v |8f33cda2c c592b4 o: 4.19| media: videobuf2-core: dequeue if start_streaming fails a |f18813d9c c592b4 .: 4.4| media: videobuf2-core: dequeue if start_streaming fails a |da6c08058 36862c .: 5.10| ARM: dts: stm32: Disable LAN8710 EDPD on DHCOM a |630677821 15f68f .: 5.10| ARM: dts: stm32: Fix touchscreen IRQ line assignment on DHCOM a |f84d7d425 7199dd .: 5.10| dmaengine: imx-dma: configure the generic DMA type to make it work a |e0188a8de d51c59 o: 5.10| net, gro: Set inner transport header offset in tcp/udp GRO hook a |72795b111 e11e86 .: 5.10| net: dsa: sja1105: overwrite dynamic FDB entries with static ones in .port_fdb_add a |0c548fae4 6c5fc1 .: 5.10| net: dsa: sja1105: invalidate dynamic FDB entries learned concurrently with statically added ones
Hi Pavel, On Fri, Aug 13, 2021 at 11:31:04AM +0200, Pavel Machek wrote: > > > If we could agree on > > > > > > Commit: (SHA) > > > > > > in the beggining of body, that would be great. > > > > > > Upstream: (SHA) > > > > > > in sign-off area would be even better. > > > > What exactly are you trying to do when you find a sha1? For some reason > > my scripts work just fine with a semi-free-form way that we currently > > have been doing this for the past 17+ years. What are you attempting to > > do that requires such a fixed format? > > Is there any problem having a fixed format? You are producing -stable > kernels, so you are not the one needing such functionality. When I was doing extended LTS in the past, I used to restart from Greg's closest branch (e.g. 4.4.y for latest 3.10.y) and needed exactly that. It was pretty easy in the end, as you'll essentially find two formats (one form from net and the other for the rest of the patches): - commit XXXX upstream - [ Upstream commit XXXX ] I ended up writing this trivial script that did the job well for me and also supported the "git cherry-pick -x" format that I was using a lot. Feel free to reuse that as a starting point, here it comes, a bit covered in dust :-) Willy #!/bin/bash die() { [ "$#" -eq 0 ] || echo "$*" >&2 exit 1 } [ -n "$1" ] || die "Usage: ${0##*/} <commit>|<file>" upstream=( ) if [ -s "$1" ]; then upstream=( $(sed -n -e '/^$/,/^./s/^commit \([0-9a-f]*\) upstream\.$/\1/p' \ -e 's/^\[ Upstream commit \([0-9a-f]*\) \]$/\1/p' \ -e 's/^(cherry picked from commit \([0-9a-f]*\))/\1/p' "$1") ) else upstream=( $(git log -1 --pretty --format=%B "$1" | \ sed -n -e '1,3s/^commit \([0-9a-f]*\) upstream\.$/\1/p' \ -e 's/^\[ Upstream commit \([0-9a-f]*\) \]$/\1/p' \ -e 's/^(cherry picked from commit \([0-9a-f]*\))/\1/p') ) fi [ "${#upstream[@]}" -gt 0 ] || die "No upstream commit ID found." echo "${upstream[0]}"
On Fri 2021-08-13 11:54:29, Willy Tarreau wrote: > Hi Pavel, > > On Fri, Aug 13, 2021 at 11:31:04AM +0200, Pavel Machek wrote: > > > > If we could agree on > > > > > > > > Commit: (SHA) > > > > > > > > in the beggining of body, that would be great. > > > > > > > > Upstream: (SHA) > > > > > > > > in sign-off area would be even better. > > > > > > What exactly are you trying to do when you find a sha1? For some reason > > > my scripts work just fine with a semi-free-form way that we currently > > > have been doing this for the past 17+ years. What are you attempting to > > > do that requires such a fixed format? > > > > Is there any problem having a fixed format? You are producing -stable > > kernels, so you are not the one needing such functionality. > > When I was doing extended LTS in the past, I used to restart from > Greg's closest branch (e.g. 4.4.y for latest 3.10.y) and needed > exactly that. It was pretty easy in the end, as you'll essentially > find two formats (one form from net and the other for the rest of > the patches): > > - commit XXXX upstream > - [ Upstream commit XXXX ] > > I ended up writing this trivial script that did the job well for me > and also supported the "git cherry-pick -x" format that I was using > a lot. Feel free to reuse that as a starting point, here it comes, a > bit covered in dust :-) Please see previous discussion. Yes, I have my regexps, too, but there are variations, and there were even false positives.. One of them is in this email thread. Greg suggests to simply ignore context and look for SHA1 sum; that does not work, either. So what I'm asking is for single, easy to parse format. I don't quite care what it is, but Upstream: (SHA) would be best. Best regards, Pavel > Willy > > #!/bin/bash > > die() { > [ "$#" -eq 0 ] || echo "$*" >&2 > exit 1 > } > > [ -n "$1" ] || die "Usage: ${0##*/} <commit>|<file>" > > upstream=( ) > if [ -s "$1" ]; then > upstream=( $(sed -n -e '/^$/,/^./s/^commit \([0-9a-f]*\) upstream\.$/\1/p' \ > -e 's/^\[ Upstream commit \([0-9a-f]*\) \]$/\1/p' \ > -e 's/^(cherry picked from commit \([0-9a-f]*\))/\1/p' "$1") ) > else > upstream=( $(git log -1 --pretty --format=%B "$1" | \ > sed -n -e '1,3s/^commit \([0-9a-f]*\) upstream\.$/\1/p' \ > -e 's/^\[ Upstream commit \([0-9a-f]*\) \]$/\1/p' \ > -e 's/^(cherry picked from commit \([0-9a-f]*\))/\1/p') ) > fi > > [ "${#upstream[@]}" -gt 0 ] || die "No upstream commit ID found." > > echo "${upstream[0]}" -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
On Fri, Aug 13, 2021 at 12:24:29PM +0200, Pavel Machek wrote: > On Fri 2021-08-13 11:54:29, Willy Tarreau wrote: > > Hi Pavel, > > > > On Fri, Aug 13, 2021 at 11:31:04AM +0200, Pavel Machek wrote: > > > > > If we could agree on > > > > > > > > > > Commit: (SHA) > > > > > > > > > > in the beggining of body, that would be great. > > > > > > > > > > Upstream: (SHA) > > > > > > > > > > in sign-off area would be even better. > > > > > > > > What exactly are you trying to do when you find a sha1? For some reason > > > > my scripts work just fine with a semi-free-form way that we currently > > > > have been doing this for the past 17+ years. What are you attempting to > > > > do that requires such a fixed format? > > > > > > Is there any problem having a fixed format? You are producing -stable > > > kernels, so you are not the one needing such functionality. > > > > When I was doing extended LTS in the past, I used to restart from > > Greg's closest branch (e.g. 4.4.y for latest 3.10.y) and needed > > exactly that. It was pretty easy in the end, as you'll essentially > > find two formats (one form from net and the other for the rest of > > the patches): > > > > - commit XXXX upstream > > - [ Upstream commit XXXX ] > > > > I ended up writing this trivial script that did the job well for me > > and also supported the "git cherry-pick -x" format that I was using > > a lot. Feel free to reuse that as a starting point, here it comes, a > > bit covered in dust :-) > > Please see previous discussion. Yes, I have my regexps, too, but there > are variations, and there were even false positives.. One of them is > in this email thread. > > Greg suggests to simply ignore context and look for SHA1 sum; that > does not work, either. The number of patches that your regex does not work on is a very tiny %, right? Can't you just handle those "by hand"? > So what I'm asking is for single, easy to parse format. I don't quite > care what it is, but As long as people end up sending us patches as backports, they will get the format wrong in odd ways over time. Heck, we can't even all get a simple signed-off-by: right all the time, look at the kernel logs for loads of issues where long-time developers mess that one up. The phrase "perfect is the enemy of good" or something like that applies here. I'm giving you backported patches "for free", the number of ones that someone messes up the text on is so small it should be lost in the noise... thanks, greg k-h
On Fri, Aug 13, 2021 at 01:02:43PM +0200, Greg Kroah-Hartman wrote: > > > I ended up writing this trivial script that did the job well for me > > > and also supported the "git cherry-pick -x" format that I was using > > > a lot. Feel free to reuse that as a starting point, here it comes, a > > > bit covered in dust :-) > > > > Please see previous discussion. Yes, I have my regexps, too, but there > > are variations, and there were even false positives.. One of them is > > in this email thread. > > > > Greg suggests to simply ignore context and look for SHA1 sum; that > > does not work, either. > > The number of patches that your regex does not work on is a very tiny %, > right? Can't you just handle those "by hand"? I agree, that's what I used to do as well and this never caused me any particular difficult. The rare cases where the script emits no ID just have to be dealt with manually. It used to happen less than once per series (with series containing ~1k input patches). I'd say that the amount of failed backports is roughly in the same order of magnitude as the ones that could be missed this way, this needs to be put in perspective! > > So what I'm asking is for single, easy to parse format. I don't quite > > care what it is, but > > As long as people end up sending us patches as backports, they will get > the format wrong in odd ways over time. Heck, we can't even all get a > simple signed-off-by: right all the time, look at the kernel logs for > loads of issues where long-time developers mess that one up. Plus this adds some cognitive load on those writing these patches, which increases the global effort. It's already difficult enough to figure the appropriate Cc list when writing a fix, let's not add more burden in this chain. > The phrase "perfect is the enemy of good" or something like that applies > here. I'm giving you backported patches "for free", the number of ones > that someone messes up the text on is so small it should be lost in the > noise... I'm also defending this on other projects. I find it important that efforts are reasonably shared. If tolerating 1% failures saves 20% effort on authors and adds 2% work on recipients, that's a net global win. You never completely eliminate mistakes anyway, regardless of the cost. Willy
On Fri, Aug 13, 2021 at 01:19:53PM +0200, Willy Tarreau wrote: > > Plus this adds some cognitive load on those writing these patches, which > increases the global effort. It's already difficult enough to figure the > appropriate Cc list when writing a fix, let's not add more burden in this > chain. > > ... > > I'm also defending this on other projects. I find it important that > efforts are reasonably shared. If tolerating 1% failures saves 20% > effort on authors and adds 2% work on recipients, that's a net global > win. You never completely eliminate mistakes anyway, regardless of the > cost. The only way I can see to square the circle would be if there was some kind of script that added enough value that people naturally use it because it saves *them* time, and it automatically inserts the right commit metadata in some kind of standardized way. I've been starting to use b4, and that's a great example of a workflow that saves me time, and standardizes things as a very nice side effect. So perhaps the question is there some kind of automation that saves 10-20% effort for authors *and* improves the quality of the patch metadata for those that choose to use the script? - Ted
On Fri, Aug 13, 2021 at 11:46:38AM -0400, Theodore Ts'o wrote: > On Fri, Aug 13, 2021 at 01:19:53PM +0200, Willy Tarreau wrote: > > > > Plus this adds some cognitive load on those writing these patches, which > > increases the global effort. It's already difficult enough to figure the > > appropriate Cc list when writing a fix, let's not add more burden in this > > chain. > > > > ... > > > > I'm also defending this on other projects. I find it important that > > efforts are reasonably shared. If tolerating 1% failures saves 20% > > effort on authors and adds 2% work on recipients, that's a net global > > win. You never completely eliminate mistakes anyway, regardless of the > > cost. > > The only way I can see to square the circle would be if there was some > kind of script that added enough value that people naturally use it > because it saves *them* time, and it automatically inserts the right > commit metadata in some kind of standardized way. > > I've been starting to use b4, and that's a great example of a workflow > that saves me time, and standardizes things as a very nice side > effect. So perhaps the question is there some kind of automation that > saves 10-20% effort for authors *and* improves the quality of the > patch metadata for those that choose to use the script? A script/tool does generate the metadata in the "correct" way, as that is what Sasha and I use. It is the issue for when people do it on their own for various reasons and do not just point us at an upstream commit that can cause issues. In those cases, people wouldn't be using any script anyway, so there's nothing really to do here. thanks, greg k-h
Hi! > > > Plus this adds some cognitive load on those writing these patches, which > > > increases the global effort. It's already difficult enough to figure the > > > appropriate Cc list when writing a fix, let's not add more burden in this > > > chain. > > > > > > ... > > > > > > I'm also defending this on other projects. I find it important that > > > efforts are reasonably shared. If tolerating 1% failures saves 20% > > > effort on authors and adds 2% work on recipients, that's a net global > > > win. You never completely eliminate mistakes anyway, regardless of the > > > cost. > > > > The only way I can see to square the circle would be if there was some > > kind of script that added enough value that people naturally use it > > because it saves *them* time, and it automatically inserts the right > > commit metadata in some kind of standardized way. > > > > I've been starting to use b4, and that's a great example of a workflow > > that saves me time, and standardizes things as a very nice side > > effect. So perhaps the question is there some kind of automation that > > saves 10-20% effort for authors *and* improves the quality of the > > patch metadata for those that choose to use the script? > > A script/tool does generate the metadata in the "correct" way, as that > is what Sasha and I use. It is the issue for when people do it on their > own for various reasons and do not just point us at an upstream commit > that can cause issues. In those cases, people wouldn't be using any > script anyway, so there's nothing really to do here. I agree that submitters would need to know about the tag; OTOH I believe that if it looked like a tag, people would be more likely to get it right. We moved from "mention what this fixes in body" to "Fixes: " and I believe that was an improvement. Anyway, three new entries in stable queues have format I have not seen before: |ec547f971 None .: 4.19| KVM: nSVM: always intercept VMLOAD/VMSAVE when nested (CVE-2021-3656) |dbfcc0f75 None o: 4.19| KVM: nSVM: avoid picking up unsupported bits from L2 in int_ctl (CVE-2021-3653) |b79b08940 None o: 4.4| KVM: nSVM: avoid picking up unsupported bits from L2 in int_ctl (CVE-2021-3653) [ upstream commit 0f923e07124df069ba68d8bb12324398f4b6b709 ] I guess I'll simply update the script. Best regards, Pavel -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Hi Pavel, On Thu, Aug 19, 2021 at 10:22:42AM +0200, Pavel Machek wrote: > I agree that submitters would need to know about the tag; OTOH I > believe that if it looked like a tag, people would be more likely to > get it right. We moved from "mention what this fixes in body" to > "Fixes: " and I believe that was an improvement. > > Anyway, three new entries in stable queues have format I have not seen > before: > > |ec547f971 None .: 4.19| KVM: nSVM: always intercept VMLOAD/VMSAVE when nested (CVE-2021-3656) > |dbfcc0f75 None o: 4.19| KVM: nSVM: avoid picking up unsupported bits from L2 in int_ctl (CVE-2021-3653) > |b79b08940 None o: 4.4| KVM: nSVM: avoid picking up unsupported bits from L2 in int_ctl (CVE-2021-3653) I can't find these commits. > [ upstream commit 0f923e07124df069ba68d8bb12324398f4b6b709 ] I've seen them a few times, mostly in the bpf subsystem or in net components whose backports were provided later by the original patch authors (or users) trying to use the same format and using a different case on "upstream". > I guess I'll simply update the script. That's clearly how it ought to be done. Again, I don't remember having faced issues during 2.6.32/3.10 in the past using the trivial script I shared and which used to ignore the case, so I don't see any particular difficulty there :-/ Cheers, Willy
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -2351,6 +2351,12 @@ static int eb_parse(struct i915_execbuff eb->batch_flags |= I915_DISPATCH_SECURE; } + batch = eb_dispatch_secure(eb, shadow); + if (IS_ERR(batch)) { + err = PTR_ERR(batch); + goto err_trampoline; + } + err = intel_engine_cmd_parser(eb->engine, eb->batch->vma, eb->batch_start_offset, @@ -2377,6 +2383,7 @@ secure_batch: err_unpin_batch: if (batch) i915_vma_unpin(batch); +err_trampoline: if (trampoline) i915_vma_unpin(trampoline); err_shadow: