Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix access to 'sock_extended_err' when generating cmsg from expressions #68

Merged
merged 1 commit into from
Nov 15, 2022

Conversation

dcaratti
Copy link
Contributor

when packetdrill builds a cmsg parsed from the script, it handles 'ee_origin', 'ee_type' and 'ee_code' as 32-bit numbers. These members of 'struct sock_extended_err' are 8-bit wide: because of this, TCP tests belonging to 'timestamping' and 'zerocopy' categories systematically fail on big endian architectures (such as s390x). For example, the value of 'ee_origin' is written in 'ee_pad':

(gdb) set args -D TFO_COOKIE=de4f234f0f433a55 -D CMSG_LEVEL_IP=SOL_IP -D CMSG_TYPE_RECVERR=IP_RECVERR basic.pkt
(gdb) r

[...]

24: system call: recvmsg
24: invoke call: recvmsg
waiting until 1668090937472983 -- now is 1668090937472985
expected: 0.000 actual: 0.000 (secs)

Thread 1 "packetdrill" hit Breakpoint 2, new_extended_err (expr=0x111e7a0, ee=0x1124d80, error=0x3ffffff9730)
at run_system_call.c:500
500 if (get_s32(expr->ee_errno, (s32 *)&ee->ee_errno, error))
(gdb) n
502 if (get_s32(expr->ee_origin, (s32 *)&ee->ee_origin, error))
(gdb) n
504 if (get_s32(expr->ee_type, (s32 *)&ee->ee_type, error))
(gdb) p *ee
$46 = {ee_errno = 0, ee_origin = 0 '\000', ee_type = 0 '\000', ee_code = 0 '\000', ee_pad = 5 '\005', ee_info = 0,
ee_data = 0}
(gdb) up
#1 0x000000000101d892 in cmsg_new (expr=0x111f740, msg=0x1124c60, error=0x3ffffff9730) at run_system_call.c:617
617 if (new_extended_err(ee_expr,
(gdb) p *(struct sock_extended_err *)data
$47 = {ee_errno = 0, ee_origin = 0 '\000', ee_type = 0 '\000', ee_code = 0 '\000', ee_pad = 5 '\005', ee_info = 0,
ee_data = 0}
(gdb)

fix it by providing a correct integer size for 'ee_origin', 'ee_type' and 'ee_code'.

Reported-by: Xiumei Mu xmu@redhat.com
Signed-off-by: Davide Caratti dcaratti@redhat.com

Copy link
Collaborator

@nealcardwell nealcardwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix!

For consistency/clarity, can you please make the summary/first-line of the commit message look like:

net-test: packetdrill: fix access to 'sock_extended_err' when generating cmsg from expressions

Thanks!

Copy link
Collaborator

@nealcardwell nealcardwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The packetdrill tree uses Linux's scripts/checkpatch.pl for style validation, and it caught some issues (see below). I will fix those issues and the commit title issue, and merge if all our internal tests pass. Thanks!

ERROR: code indent should use tabs where possible
#77: FILE: gtests/net/packetdrill/run_system_call.c:206:

  •             s8 *value, char **error)$
    

WARNING: please, no spaces at the start of a line
#77: FILE: gtests/net/packetdrill/run_system_call.c:206:

  •             s8 *value, char **error)$
    

WARNING: please, no spaces at the start of a line
#79: FILE: gtests/net/packetdrill/run_system_call.c:208:

  •   if (check_type(expression, EXPR_INTEGER, error))$
    

WARNING: suspect code indent for conditional statements (7, 15)
#79: FILE: gtests/net/packetdrill/run_system_call.c:208:

  •   if (check_type(expression, EXPR_INTEGER, error))
    
  •           return STATUS_ERR;
    

ERROR: code indent should use tabs where possible
#80: FILE: gtests/net/packetdrill/run_system_call.c:209:

  •           return STATUS_ERR;$
    

WARNING: please, no spaces at the start of a line
#80: FILE: gtests/net/packetdrill/run_system_call.c:209:

  •           return STATUS_ERR;$
    

WARNING: please, no spaces at the start of a line
#81: FILE: gtests/net/packetdrill/run_system_call.c:210:

  •   if ((expression->value.num > UCHAR_MAX) ||$
    

WARNING: suspect code indent for conditional statements (7, 15)
#81: FILE: gtests/net/packetdrill/run_system_call.c:210:

  •   if ((expression->value.num > UCHAR_MAX) ||
    

[...]

  •           asprintf(error,
    

ERROR: code indent should use tabs where possible
#82: FILE: gtests/net/packetdrill/run_system_call.c:211:

  •       (expression->value.num < CHAR_MIN)) {$
    

WARNING: please, no spaces at the start of a line
#82: FILE: gtests/net/packetdrill/run_system_call.c:211:

  •       (expression->value.num < CHAR_MIN)) {$
    

ERROR: code indent should use tabs where possible
#83: FILE: gtests/net/packetdrill/run_system_call.c:212:

  •           asprintf(error,$
    

WARNING: please, no spaces at the start of a line
#83: FILE: gtests/net/packetdrill/run_system_call.c:212:

  •           asprintf(error,$
    

ERROR: code indent should use tabs where possible
#84: FILE: gtests/net/packetdrill/run_system_call.c:213:

  •                    "Value out of range for 8-bit integer: %lld",$
    

WARNING: please, no spaces at the start of a line
#84: FILE: gtests/net/packetdrill/run_system_call.c:213:

  •                    "Value out of range for 8-bit integer: %lld",$
    

ERROR: code indent should use tabs where possible
#85: FILE: gtests/net/packetdrill/run_system_call.c:214:

  •                    expression->value.num);$
    

WARNING: please, no spaces at the start of a line
#85: FILE: gtests/net/packetdrill/run_system_call.c:214:

  •                    expression->value.num);$
    

ERROR: code indent should use tabs where possible
#86: FILE: gtests/net/packetdrill/run_system_call.c:215:

  •           return STATUS_ERR;$
    

WARNING: please, no spaces at the start of a line
#86: FILE: gtests/net/packetdrill/run_system_call.c:215:

  •           return STATUS_ERR;$
    

WARNING: please, no spaces at the start of a line
#87: FILE: gtests/net/packetdrill/run_system_call.c:216:

  •   }$
    

WARNING: please, no spaces at the start of a line
#88: FILE: gtests/net/packetdrill/run_system_call.c:217:

  •   *value = expression->value.num;$
    

WARNING: please, no spaces at the start of a line
#89: FILE: gtests/net/packetdrill/run_system_call.c:218:

  •   return STATUS_OK;$
    

total: 7 errors, 15 warnings, 40 lines checked

…ing cmsg from expressions

when packetdrill builds a cmsg parsed from the script, it handles
'ee_origin', 'ee_type' and 'ee_code' as 32-bit numbers. These members of
'struct sock_extended_err' are 8-bit wide: because of this, TCP tests
belonging to 'timestamping' and 'zerocopy' categories systematically fail
on big endian architectures (such as s390x). For example, the value of
'ee_origin' is written in 'ee_pad':

 (gdb) set args -D TFO_COOKIE=de4f234f0f433a55 -D CMSG_LEVEL_IP=SOL_IP -D CMSG_TYPE_RECVERR=IP_RECVERR basic.pkt
 (gdb) r

[...]

 24: system call: recvmsg
 24: invoke call: recvmsg
 waiting until 1668090937472983 -- now is 1668090937472985
 expected: 0.000 actual: 0.000  (secs)

 Thread 1 "packetdrill" hit Breakpoint 2, new_extended_err (expr=0x111e7a0, ee=0x1124d80, error=0x3ffffff9730)
     at run_system_call.c:500
 500             if (get_s32(expr->ee_errno, (s32 *)&ee->ee_errno, error))
 (gdb) n
 502             if (get_s32(expr->ee_origin, (s32 *)&ee->ee_origin, error))
 (gdb) n
 504             if (get_s32(expr->ee_type, (s32 *)&ee->ee_type, error))
 (gdb) p *ee
 $46 = {ee_errno = 0, ee_origin = 0 '\000', ee_type = 0 '\000', ee_code = 0 '\000', ee_pad = 5 '\005', ee_info = 0,
  ee_data = 0}
 (gdb) up
 #1  0x000000000101d892 in cmsg_new (expr=0x111f740, msg=0x1124c60, error=0x3ffffff9730) at run_system_call.c:617
 617                             if (new_extended_err(ee_expr,
 (gdb) p *(struct sock_extended_err *)data
 $47 = {ee_errno = 0, ee_origin = 0 '\000', ee_type = 0 '\000', ee_code = 0 '\000', ee_pad = 5 '\005', ee_info = 0,
   ee_data = 0}
 (gdb)

fix it by providing a correct integer size for 'ee_origin', 'ee_type' and 'ee_code'.

Reported-by: Xiumei Mu <xmu@redhat.com>
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
@dcaratti dcaratti force-pushed the fix-sock_extended_err-s390x branch from 5743345 to 9676c0d Compare November 15, 2022 09:02
@dcaratti
Copy link
Contributor Author

Thanks for the fix!

For consistency/clarity, can you please make the summary/first-line of the commit message look like:

net-test: packetdrill: fix access to 'sock_extended_err' when generating cmsg from expressions

Thanks!

hi Neal, I just fixed the commit message. Thanks for reviewing!

@dcaratti
Copy link
Contributor Author

The packetdrill tree uses Linux's scripts/checkpatch.pl for style validation, and it caught some issues (see below).

Ouch, sorry for the copy/paste noise.

I will fix those issues and the commit title issue, and merge if all our internal tests pass. Thanks!

Ok; for your convenience, I refreshed the PR with fixed indentation. Thanks!

Copy link
Collaborator

@nealcardwell nealcardwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks!

@nealcardwell nealcardwell merged commit 1dcaaca into google:master Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants