RHEL Engineering is moving the tracking of its product development work on RHEL 6 through RHEL 9 to Red Hat Jira (issues.redhat.com). If you're a Red Hat customer, please continue to file support cases via the Red Hat customer portal. If you're not, please head to the "RHEL project" in Red Hat Jira and file new tickets here. Individual Bugzilla bugs in the statuses "NEW", "ASSIGNED", and "POST" are being migrated throughout September 2023. Bugs of Red Hat partners with an assigned Engineering Partner Manager (EPM) are migrated in late September as per pre-agreed dates. Bugs against components "kernel", "kernel-rt", and "kpatch" are only migrated if still in "NEW" or "ASSIGNED". If you cannot log in to RH Jira, please consult article #7032570. That failing, please send an e-mail to the RH Jira admins at rh-issues@redhat.com to troubleshoot your issue as a user management inquiry. The email creates a ServiceNow ticket with Red Hat. Individual Bugzilla bugs that are migrated will be moved to status "CLOSED", resolution "MIGRATED", and set with "MigratedToJIRA" in "Keywords". The link to the successor Jira issue will be found under "Links", have a little "two-footprint" icon next to it, and direct you to the "RHEL project" in Red Hat Jira (issue links are of type "https://issues.redhat.com/browse/RHEL-XXXX", where "X" is a digit). This same link will be available in a blue banner at the top of the page informing you that that bug has been migrated.
Bug 1322406 - TestOnly: Backport recent pedit action fixes from upstream
Summary: TestOnly: Backport recent pedit action fixes from upstream
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: Red Hat Enterprise Linux 7
Classification: Red Hat
Component: iproute
Version: 7.3
Hardware: All
OS: Linux
medium
medium
Target Milestone: rc
: ---
Assignee: Andrea Claudi
QA Contact: BaseOS QE Security Team
URL:
Whiteboard:
Depends On: 1435647
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-03-30 12:41 UTC by Phil Sutter
Modified: 2020-03-02 17:29 UTC (History)
7 users (show)

Fixed In Version: iproute-4.11.0-4.el7
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-03-02 17:14:53 UTC
Target Upstream Version:
Embargoed:

Attachments (Terms of Use)

Description Phil Sutter 2016-03-30 12:41:57 UTC 
The pedit tc action is severely broken in RHEL, as it was upstream until recently. The following fixes should be beackported:

commit ec0ceeec4954b1a5439ec3684460e8385454de90
Author: Phil Sutter <phil>
Date:   Wed Mar 2 12:20:29 2016 +0100

    tc: pedit: Fix layered op parsing
    
    After lookup of the layered op submodule, pedit would pass argv and argc
    including the layered op identifier at first position which confused the
    submodule parser. Fix this by calling NEXT_ARG() before calling the
    parse_peopt() callback.
    
    Signed-off-by: Phil Sutter <phil>

commit f440e9d8c2f6f0eb2e9fccd8f4d7c42c11ba4979
Author: Phil Sutter <phil>
Date:   Wed Mar 2 12:20:30 2016 +0100

    tc: pedit: Fix parse_cmd()
    
    This was horribly broken:
    * pack_key8() and pack_key16() ...
      * missed to invert retain value when applying it to the mask,
      * did not sanitize val by ANDing it with retain,
      * and ignored the mask which is necessary for 'invert' command.
    * pack_key16() did not convert mask to network byte order.
    * Changing the retain value for 'invert' or 'retain' operation seems
      just plain wrong.
    * While here, also got rid of unnecessary offset sanitization in
      pack_key32().
    * Simplify code a bit by always assigning the local mask variable to
      tkey->mask before calling any of the pack_key*() variants.
    
    Signed-off-by: Phil Sutter <phil>

commit 338b003bcc22a62c98b84dbe5e491cae84dbb03c
Author: Phil Sutter <phil>
Date:   Wed Mar 2 12:20:31 2016 +0100

    tc: pedit: Fix retain value for ihl adjustments
    
    Since the IP Header Length field is just half a byte, adjust retain to
    only match these bits so the Version field is not overwritten by
    accident.
    
    The whole concept is actually broken due to dependency on endianness
    which pedit ignores.
    
    Signed-off-by: Phil Sutter <phil>

commit 77bed404d03f39adf2842ee4aefb53ba8a68f087
Author: Phil Sutter <phil>
Date:   Tue Mar 22 15:16:22 2016 +0100

    tc: pedit: Fix for big-endian systems
    
    This was tricky to get right:
    - The 'stride' value used for 8 and 16 bit values must behave inverse to
      the value's intra word offset to work correctly with big-endian data
      act_pedit is editing.
    - The 'm' array's values are in host byte order, so they have to be
      converted as well (and the ordering was just inverse, for some
      reason).
    - The only sane way of getting this right is to manipulate value/mask in
      host byte order and convert the output.
    - TIPV4 (i.e. 'munge ip src/dst') had it's own pitfall: the address
      parser converts to network byte order automatically. This patch fixes
      this by converting it back before calling pack_key32, which is a hack
      but at least does not require to implement a completely separate code
      flow.
    
    Signed-off-by: Phil Sutter <phil>
    Signed-off-by: Stephen Hemminger <stephen>

commit a33786b582a29a940cfe1eff826ff5a0548a6d81
Author: Phil Sutter <phil>
Date:   Tue Mar 22 15:16:23 2016 +0100

    tc: pedit: Fix raw op
    
    The retain value was wrong for u16 and u8 types.
    
    Signed-off-by: Phil Sutter <phil>
    Signed-off-by: Stephen Hemminger <stephen>
Comment 3 Phil Sutter 2017-01-27 11:19:21 UTC 
Re-adding devel_ack+ since I still think it should be fixed.
Comment 4 Timothy Redaelli 2017-05-15 14:33:00 UTC 
Backported all the requested commits:

commit ec0ceeec4954b1a5439ec3684460e8385454de90
commit f440e9d8c2f6f0eb2e9fccd8f4d7c42c11ba4979
commit 338b003bcc22a62c98b84dbe5e491cae84dbb03c
commit 77bed404d03f39adf2842ee4aefb53ba8a68f087
commit a33786b582a29a940cfe1eff826ff5a0548a6d81

Plus one dependent commit:

commit 0bbca0422f9779cc4eeaf70aa01dcad10d6ab076
Author: Maciej Żenczykowski <maze>
Date:   Thu Jun 25 02:03:02 2015 -0700

    iproute2: tc/m_pedit.c - remove dead code
    
    The initializers are simply not needed.
    
    These if-blocks are outright dead code, because '0 > unsigned' is always
    false, so only else clause triggers and regardless of which clause triggers
    it only updates 'ind' which is later unconditionally written to before
    being used anyway.
    
    Otherwise we get errors from clang:
    
      m_pedit.c:166:8: error: comparison of 0 > unsigned expression is always false [-Werror,-Wtautological-compare]
        if (0 > tkey->off) {
            ~ ^ ~~~~~~~~~
      m_pedit.c:209:8: error: comparison of 0 > unsigned expression is always false [-Werror,-Wtautological-compare]
        if (0 > tkey->off) {
            ~ ^ ~~~~~~~~~
      2 errors generated.
    
    Change-Id: I3c9e9092915088fc56f992e5df736851541a4458
Comment 6 Andrea Claudi 2019-07-17 13:08:15 UTC 
Hi Dalibor, this ticket is a TestOnly one, which means no action required from devel side (apart from providing devel_ack+) but testing by QA is still missing. So whenever they get capacity for it, they should provide qa_ack+ and it will make it into the next errata and is tested for. Therefore we want to keep this ticket, otherwise the feature is there but we can't (officially) be sure it's working as intended. I'll anyway move this to rhel-7.8.
Note You need to log in before you can comment on or make changes to this bug.