Bug 232192 - Harmonize udp_input() and udp6_input() locking; should make the udp6 code simpler
Summary: Harmonize udp_input() and udp6_input() locking; should make the udp6 code sim...
Status: Open
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Bjoern A. Zeeb
URL:
Keywords: feature
Depends on:
Blocks: 232348
  Show dependency treegraph
 
Reported: 2018-10-11 23:58 UTC by Bjoern A. Zeeb
Modified: 2021-02-16 22:08 UTC (History)
3 users (show)

See Also:


Attachments
screenshot from ipmi (836.73 KB, image/png)
2021-01-11 11:56 UTC, Andrey V. Elsukov
no flags Details
proposed patch (untested) (1.58 KB, patch)
2021-01-12 14:41 UTC, Andrey V. Elsukov
no flags Details | Diff
proposed patch (2.86 KB, patch)
2021-01-28 09:15 UTC, Andrey V. Elsukov
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bjoern A. Zeeb freebsd_committer 2018-10-11 23:58:27 UTC
Check if we need to apply these two commits from udp6_input() also to IPv4
( after https://reviews.freebsd.org/D17525 ) is in.

https://svnweb.freebsd.org/base?view=revision&revision=335919
https://svnweb.freebsd.org/base?view=revision&revision=335958
Comment 1 Bjoern A. Zeeb freebsd_committer 2018-10-12 21:21:29 UTC
The locking is a lot simpler in legacy IP, so we don't have to apply the changes 1:1 but can get away with a single check.
Comment 2 Bjoern A. Zeeb freebsd_committer 2018-10-12 21:34:05 UTC
https://reviews.freebsd.org/D17540



XXX TODO: harmonize IPv6 and IPv4 locking and code flow.
Comment 3 commit-hook freebsd_committer 2018-10-12 22:52:25 UTC
A commit references this bug:

Author: bz
Date: Fri Oct 12 22:51:45 UTC 2018
New revision: 339339
URL: https://svnweb.freebsd.org/changeset/base/339339

Log:
  In udp_input() when walking the pcblist we can come across
  an inp marked FREED after the epoch(9) changes.
  Check once we hold the lock and skip the inp if it is the case.

  Contrary to IPv6 the locking of the inp is outside the multicast
  section and hence a single check seems to suffice.

  PR:		232192
  Reviewed by:	mmacy, markj
  Approved by:	re (kib)
  Differential Revision:	https://reviews.freebsd.org/D17540

Changes:
  head/sys/netinet/udp_usrreq.c
Comment 4 Andrey V. Elsukov freebsd_committer 2021-01-11 11:56:02 UTC
Created attachment 221454 [details]
screenshot from ipmi

Hi Bjoern,

it seems there is still possible NULL pointer dereference panic in the related code. PCB is currently protected by NET_EPOCH section, and we can safely make access to PCB while holding INP_RLOCK().
But access to inp->inp_socket is not safe without the lock. 
I attached screenshot of panic, unfortunately there is no core dump. From the kgdb I obtined the line number, where the panic occured:

 415                         if ((last->inp_socket->so_options &
 416                              (SO_REUSEPORT|SO_REUSEPORT_LB|SO_REUSEADDR)) == 0)
 417                                 break;
Comment 5 Andrey V. Elsukov freebsd_committer 2021-01-12 14:41:33 UTC
Created attachment 221493 [details]
proposed patch (untested)

Probably, this patch should fix the problem. I'll test it for some time, then report back.
Comment 6 Andrey V. Elsukov freebsd_committer 2021-01-28 09:09:07 UTC
I have got the core dump:

Fatal trap 12: page fault while in kernel mode
cpuid = 8; apic id = 12
fault virtual address	= 0xcc
fault code		= supervisor read data, page not present
instruction pointer	= 0x20:0xffffffff80d36a19
stack pointer	        = 0x0:0xfffffe015b813440
frame pointer	        = 0x0:0xfffffe015b813560
code segment		= base rx0, limit 0xfffff, type 0x1b
			= DPL 0, pres 1, long 1, def32 0, gran 1
processor eflags	= interrupt enabled, resume, IOPL = 0
current process		= 12 (irq141: mlx5_core1)
trap number		= 12
panic: page fault
cpuid = 8
time = 1611737705
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xfffffe015b813100
vpanic() at vpanic+0x182/frame 0xfffffe015b813150
panic() at panic+0x43/frame 0xfffffe015b8131b0
trap_fatal() at trap_fatal+0x387/frame 0xfffffe015b813210
trap_pfault() at trap_pfault+0x4f/frame 0xfffffe015b813260
trap() at trap+0x271/frame 0xfffffe015b813370
calltrap() at calltrap+0x8/frame 0xfffffe015b813370
--- trap 0xc, rip = 0xffffffff80d36a19, rsp = 0xfffffe015b813440, rbp = 0xfffffe015b813560 ---
udp6_input() at udp6_input+0x759/frame 0xfffffe015b813560
ip6_input() at ip6_input+0xb3a/frame 0xfffffe015b813640
netisr_dispatch_src() at netisr_dispatch_src+0xca/frame 0xfffffe015b813690
ether_demux() at ether_demux+0x138/frame 0xfffffe015b8136c0
ether_nh_input() at ether_nh_input+0x344/frame 0xfffffe015b813720
netisr_dispatch_src() at netisr_dispatch_src+0xca/frame 0xfffffe015b813770
ether_input() at ether_input+0x69/frame 0xfffffe015b8137d0
ether_demux() at ether_demux+0x121/frame 0xfffffe015b813800
ether_nh_input() at ether_nh_input+0x344/frame 0xfffffe015b813860
netisr_dispatch_src() at netisr_dispatch_src+0xca/frame 0xfffffe015b8138b0
ether_input() at ether_input+0x69/frame 0xfffffe015b813910
tcp_lro_queue_mbuf() at tcp_lro_queue_mbuf+0xca/frame 0xfffffe015b813940
mlx5e_rx_cq_comp() at mlx5e_rx_cq_comp+0xfd/frame 0xfffffe015b813a60
mlx5_cq_completion() at mlx5_cq_completion+0x90/frame 0xfffffe015b813ac0
mlx5_eq_int() at mlx5_eq_int+0xb0/frame 0xfffffe015b813b10
mlx5_msix_handler() at mlx5_msix_handler+0x15/frame 0xfffffe015b813b20
ithread_loop() at ithread_loop+0x24d/frame 0xfffffe015b813bb0
fork_exit() at fork_exit+0x7e/frame 0xfffffe015b813bf0
fork_trampoline() at fork_trampoline+0xe/frame 0xfffffe015b813bf0
--- trap 0, rip = 0, rsp = 0, rbp = 0 ---
KDB: enter: panic

__curthread () at /usr/src/sys/amd64/include/pcpu_aux.h:55
55	/usr/src/sys/amd64/include/pcpu_aux.h: No such file or directory.

(kgdb) bt
#0  __curthread () at /usr/src/sys/amd64/include/pcpu_aux.h:55
#1  doadump (textdump=3670034) at /usr/src/sys/kern/kern_shutdown.c:394
#2  0xffffffff8047584e in db_fncall_generic (addr=<optimized out>, nargs=0, args=<optimized out>, rv=<optimized out>) at /usr/src/sys/ddb/db_command.c:610
#3  db_fncall (dummy1=<optimized out>, dummy2=<optimized out>, dummy3=<optimized out>, dummy4=<optimized out>) at /usr/src/sys/ddb/db_command.c:658
#4  0xffffffff8047537c in db_command (last_cmdp=<optimized out>, cmd_table=<optimized out>, dopager=1) at /usr/src/sys/ddb/db_command.c:482
#5  0xffffffff804750ed in db_command_loop () at /usr/src/sys/ddb/db_command.c:535
#6  0xffffffff80478398 in db_trap (type=<optimized out>, code=<optimized out>) at /usr/src/sys/ddb/db_main.c:270
#7  0xffffffff80b85f24 in kdb_trap (type=3, code=0, tf=<optimized out>) at /usr/src/sys/kern/subr_kdb.c:699
#8  0xffffffff80efa6bc in trap (frame=0xfffffe015b813030) at /usr/src/sys/amd64/amd64/trap.c:576
#9  <signal handler called>
#10 kdb_enter (why=0xffffffff810156ac "panic", msg=<optimized out>) at /usr/src/sys/kern/subr_kdb.c:486
#11 0xffffffff80b37b1e in vpanic (fmt=<optimized out>, ap=<optimized out>) at /usr/src/sys/kern/kern_shutdown.c:902
#12 0xffffffff80b37973 in panic (fmt=0xffffffff814c7230 <gdb_consdev> " S8\201\377\377\377\377\001") at /usr/src/sys/kern/kern_shutdown.c:839
#13 0xffffffff80efab37 in trap_fatal (frame=0xfffffe015b813380, eva=204) at /usr/src/sys/amd64/amd64/trap.c:915
#14 0xffffffff80efab8f in trap_pfault (frame=0xfffffe015b813380, usermode=<optimized out>, signo=<optimized out>, ucode=<optimized out>)
    at /usr/src/sys/amd64/amd64/trap.c:732
#15 0xffffffff80efa1f1 in trap (frame=0xfffffe015b813380) at /usr/src/sys/amd64/amd64/trap.c:398
#16 <signal handler called>
#17 udp6_input (mp=0xfffffe015b8135a8, offp=<optimized out>, proto=<optimized out>) at /usr/src/sys/netinet6/udp6_usrreq.c:418
#18 0xffffffff80d1caca in ip6_input (m=0xfffff80325811600) at /usr/src/sys/netinet6/ip6_input.c:931
#19 0xffffffff80c5be5a in netisr_dispatch_src (proto=6, source=<optimized out>, m=0xffffffff815742c0 <vnet_entry_udb>) at /usr/src/sys/net/netisr.c:1143
#20 0xffffffff80c4f548 in ether_demux (ifp=0xfffff80003d6b800, m=0x11) at /usr/src/sys/net/if_ethersubr.c:921
#21 0xffffffff80c508a4 in ether_input_internal (ifp=0xfffff80003d6b800, m=0x11) at /usr/src/sys/net/if_ethersubr.c:705
#22 ether_nh_input (m=<optimized out>) at /usr/src/sys/net/if_ethersubr.c:735
#23 0xffffffff80c5be5a in netisr_dispatch_src (proto=5, source=<optimized out>, m=0xffffffff815742c0 <vnet_entry_udb>) at /usr/src/sys/net/netisr.c:1143
#24 0xffffffff80c4f999 in ether_input (ifp=<optimized out>, m=0xfffff80325811600) at /usr/src/sys/net/if_ethersubr.c:828
#25 0xffffffff80c4f531 in ether_demux (ifp=0xfffff8090668a000, m=0x11) at /usr/src/sys/net/if_ethersubr.c:874
#26 0xffffffff80c508a4 in ether_input_internal (ifp=0xfffff8090668a000, m=0x11) at /usr/src/sys/net/if_ethersubr.c:705
#27 ether_nh_input (m=<optimized out>) at /usr/src/sys/net/if_ethersubr.c:735
#28 0xffffffff80c5be5a in netisr_dispatch_src (proto=5, source=<optimized out>, m=0xffffffff815742c0 <vnet_entry_udb>) at /usr/src/sys/net/netisr.c:1143
#29 0xffffffff80c4f999 in ether_input (ifp=<optimized out>, m=0xfffff80325811600) at /usr/src/sys/net/if_ethersubr.c:828
#30 0xffffffff80ce524a in tcp_lro_queue_mbuf (lc=0xfffffe0159c19840, mb=0xfffff80325811600) at /usr/src/sys/netinet/tcp_lro.c:1429
#31 0xffffffff820f0f2d in mlx5e_poll_rx_cq (rq=<optimized out>, budget=<error reading variable: Cannot access memory at address 0x100>)
    at /usr/src/sys/dev/mlx5/mlx5_en/mlx5_en_rx.c:521
#32 mlx5e_rx_cq_comp (mcq=<optimized out>) at /usr/src/sys/dev/mlx5/mlx5_en/mlx5_en_rx.c:572
#33 0xffffffff82120780 in mlx5_cq_completion (dev=0xfffffe01586c0000, cqn=33) at /usr/src/sys/dev/mlx5/mlx5_core/mlx5_cq.c:81
#34 0xffffffff82122a40 in mlx5_eq_int (dev=0xfffffe01586c0000, eq=0xfffff808b5a20b00) at /usr/src/sys/dev/mlx5/mlx5_core/mlx5_eq.c:250
#35 0xffffffff82122195 in mlx5_msix_handler (irq=<optimized out>, eq_ptr=0x11) at /usr/src/sys/dev/mlx5/mlx5_core/mlx5_eq.c:405
#36 0xffffffff80af8e7d in intr_event_execute_handlers (p=<optimized out>, ie=0xfffff80881b35a00) at /usr/src/sys/kern/kern_intr.c:1168
#37 ithread_execute_handlers (p=<optimized out>, ie=0xfffff80881b35a00) at /usr/src/sys/kern/kern_intr.c:1181
#38 ithread_loop (arg=0xfffff80902bbeee0) at /usr/src/sys/kern/kern_intr.c:1269
#39 0xffffffff80af592e in fork_exit (callout=0xffffffff80af8c30 <ithread_loop>, arg=0xfffff80902bbeee0, frame=0xfffffe015b813c00)
    at /usr/src/sys/kern/kern_fork.c:1052
#40 <signal handler called>

kgdb) f 17
#17 udp6_input (mp=0xfffffe015b8135a8, offp=<optimized out>, proto=<optimized out>) at /usr/src/sys/netinet6/udp6_usrreq.c:418
418	/usr/src/sys/netinet6/udp6_usrreq.c: No such file or directory.
(kgdb) i lo
inp_locked = false
pcblist = <optimized out>
last = 0xfffff809753377e0
imo = <optimized out>
fromsa = {{sin6_len = 28 '\034', sin6_family = 28 '\034', sin6_port = 56110, sin6_flowinfo = 0, sin6_addr = {__u6_addr = {
        __u6_addr8 = "\376\200", '\000' <repeats 12 times>, <incomplete sequence \365>, __u6_addr16 = {33022, 0, 0, 0, 0, 0, 0, 62720}, __u6_addr32 = {33022, 0, 0, 
          4110417920}}}, sin6_scope_id = 12}, {sin6_len = 28 '\034', sin6_family = 28 '\034', sin6_port = 56110, sin6_flowinfo = 0, sin6_addr = {__u6_addr = {
        __u6_addr8 = "\377\002", '\000' <repeats 12 times>, "\002\020", __u6_addr16 = {767, 0, 0, 0, 0, 0, 0, 4098}, __u6_addr32 = {767, 0, 0, 268566528}}}, 
    sin6_scope_id = 12}}
m = 0xfffff80325811600
off = <optimized out>
ifp = <optimized out>
uh = <optimized out>
ip6 = <optimized out>
plen = <optimized out>
nxt = <optimized out>
ulen = <optimized out>
cscov_partial = 629216880
uh_sum = <optimized out>
pcbinfo = <optimized out>
fwd_tag = <optimized out>
inp = 0xfffff809753377e0
up = <optimized out>
(kgdb) p *last
$1 = {inp_hash = {cle_next = 0xfffff809ef691000, cle_prev = 0xfffff808817ce6d8}, inp_pcbgrouphash = {cle_next = 0x0, cle_prev = 0x0}, inp_lock = {lock_object = {
      lo_name = 0xffffffff80f82e4c "udpinp", lo_flags = 90898432, lo_data = 0, lo_witness = 0x0}, rw_lock = 1}, inp_hpts = {tqe_next = 0x0, tqe_prev = 0x0}, 
  inp_hpts_request = 0, inp_in_hpts = 0 '\000', inp_in_input = 0 '\000', inp_hpts_cpu = 0, inp_refcount = 1, inp_flags = 8421376, inp_flags2 = 16, inp_input_cpu = 0, 
  inp_hpts_cpu_set = 0 '\000', inp_input_cpu_set = 0 '\000', inp_hpts_calls = 0 '\000', inp_input_calls = 0 '\000', inp_spare_bits2 = 0 '\000', 
  inp_numa_domain = 255 '\377', inp_ppcb = 0xfffff8097534fb00, inp_socket = 0x0, inp_hptsslot = 0, inp_hpts_drop_reas = 0, inp_input = {tqe_next = 0x0, 
    tqe_prev = 0x0}, inp_pcbinfo = 0xfffffe006bde1ab0, inp_pcbgroup = 0x0, inp_pcbgroup_wild = {cle_next = 0x0, cle_prev = 0x0}, inp_cred = 0xfffff80ebd25fe00, 
  inp_flow = 0, inp_vflag = 6 '\006', inp_ip_ttl = 64 '@', inp_ip_p = 0 '\000', inp_ip_minttl = 0 '\000', inp_flowid = 0, inp_snd_tag = 0x0, inp_flowtype = 0, 
  inp_rss_listen_bucket = 0, inp_inc = {inc_flags = 0 '\000', inc_len = 0 '\000', inc_fibnum = 0, inc_ie = {ie_fport = 0, ie_lport = 56110, ie_dependfaddr = {
        id46_addr = {ia46_pad32 = {0, 0, 0}, ia46_addr4 = {s_addr = 0}}, id6_addr = {__u6_addr = {__u6_addr8 = '\000' <repeats 15 times>, __u6_addr16 = {0, 0, 0, 0, 
              0, 0, 0, 0}, __u6_addr32 = {0, 0, 0, 0}}}}, ie_dependladdr = {id46_addr = {ia46_pad32 = {0, 0, 0}, ia46_addr4 = {s_addr = 0}}, id6_addr = {__u6_addr = {
            __u6_addr8 = '\000' <repeats 15 times>, __u6_addr16 = {0, 0, 0, 0, 0, 0, 0, 0}, __u6_addr32 = {0, 0, 0, 0}}}}, ie6_zoneid = 0}}, inp_label = 0x0, 
  inp_sp = 0xfffff80cf782bc60, inp_lbstates = {clh_first = 0x0}, inp_lbscnt = 0, {inp_ip_tos = 0 '\000', inp_options = 0x0, inp_moptions = 0x0}, {in6p_options = 0x0, 
    in6p_outputopts = 0x0, in6p_moptions = 0x0, in6p_icmp6filt = 0x0, in6p_cksum = -1, in6p_hops = -1}, inp_portlist = {cle_next = 0xfffff809ef691000, 
    cle_prev = 0xfffff80900259020}, inp_phd = 0xfffff80900259000, inp_gencnt = 1172298, spare_ptr = 0x0, inp_rt_cookie = 0, {inp_route = {ro_nh = 0x0, ro_lle = 0x0, 
      ro_prepend = 0x0, ro_plen = 0, ro_flags = 256, ro_mtu = 0, spare = 0, ro_dst = {sa_len = 0 '\000', sa_family = 0 '\000', sa_data = '\000' <repeats 13 times>}}, 
    inp_route6 = {ro_nh = 0x0, ro_lle = 0x0, ro_prepend = 0x0, ro_plen = 0, ro_flags = 256, ro_mtu = 0, spare = 0, ro_dst = {sin6_len = 0 '\000', 
        sin6_family = 0 '\000', sin6_port = 0, sin6_flowinfo = 0, sin6_addr = {__u6_addr = {__u6_addr8 = '\000' <repeats 15 times>, __u6_addr16 = {0, 0, 0, 0, 0, 0, 
              0, 0}, __u6_addr32 = {0, 0, 0, 0}}}, sin6_scope_id = 0}}}, inp_list = {cle_next = 0xfffff8098c1ea5e8, cle_prev = 0xfffffe006bde1ba0}, inp_epoch_ctx = {
    data = {0xffffffff80cc7f90 <in_pcbfree_deferred>, 0xfffff8098c1e63e8}}}
(kgdb) p/x last->inp_flags2
$2 = 0x10
(kgdb) p/x last->in6p_moptions
$3 = 0x0
(kgdb) p/x last->inp_socket
$4 = 0x0
(kgdb) p offsetof (struct socket, so_options)
$1 = (int *) 0xcc
Comment 7 Andrey V. Elsukov freebsd_committer 2021-01-28 09:15:36 UTC
Created attachment 221983 [details]
proposed patch
Comment 8 commit-hook freebsd_committer 2021-02-11 09:24:46 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=3c782d9c91666886d868426f0aea040d1a1a8ee4

commit 3c782d9c91666886d868426f0aea040d1a1a8ee4
Author:     Andrey V. Elsukov <ae@FreeBSD.org>
AuthorDate: 2021-02-11 08:38:41 +0000
Commit:     Andrey V. Elsukov <ae@FreeBSD.org>
CommitDate: 2021-02-11 09:00:25 +0000

    [udp6] fix possible panic due to lack of locking.

    The lookup for a IPv6 multicast addresses corresponding to
    the destination address in the datagram is protected by the
    NET_EPOCH section. Access to each PCB is protected by INP_RLOCK
    during comparing. But access to socket's so_options field is
    not protected. And in some cases it is possible, that PCB
    pointer is still valid, but inp_socket is not. The patch wides
    lock holding to protect access to inp_socket. It copies locking
    strategy from IPv4 UDP handling.

    PR:     232192
    Obtained from:  Yandex LLC
    MFC after:      3 days
    Sponsored by:   Yandex LLC
    Differential Revision:  https://reviews.freebsd.org/D28232

 sys/netinet6/udp6_usrreq.c | 61 +++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 33 deletions(-)
Comment 9 commit-hook freebsd_committer 2021-02-15 10:52:00 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=8cb4c363163062bceec92383eae43f5a32049c73

commit 8cb4c363163062bceec92383eae43f5a32049c73
Author:     Andrey V. Elsukov <ae@FreeBSD.org>
AuthorDate: 2021-02-11 08:38:41 +0000
Commit:     Andrey V. Elsukov <ae@FreeBSD.org>
CommitDate: 2021-02-15 10:48:56 +0000

    [udp6] fix possible panic due to lack of locking.

    The lookup for a IPv6 multicast addresses corresponding to
    the destination address in the datagram is protected by the
    NET_EPOCH section. Access to each PCB is protected by INP_RLOCK
    during comparing. But access to socket's so_options field is
    not protected. And in some cases it is possible, that PCB
    pointer is still valid, but inp_socket is not. The patch wides
    lock holding to protect access to inp_socket. It copies locking
    strategy from IPv4 UDP handling.

    PR:     232192
    Obtained from:  Yandex LLC
    Sponsored by:   Yandex LLC
    Differential Revision:  https://reviews.freebsd.org/D28232

    (cherry picked from commit 3c782d9c91666886d868426f0aea040d1a1a8ee4)

 sys/netinet6/udp6_usrreq.c | 61 +++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 33 deletions(-)
Comment 10 commit-hook freebsd_committer 2021-02-16 22:08:23 UTC
A commit in branch releng/13.0 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=6666b1d45a1b0ab4c31f621e6b406ff57431c91a

commit 6666b1d45a1b0ab4c31f621e6b406ff57431c91a
Author:     Andrey V. Elsukov <ae@FreeBSD.org>
AuthorDate: 2021-02-11 08:38:41 +0000
Commit:     Andrey V. Elsukov <ae@FreeBSD.org>
CommitDate: 2021-02-16 22:03:08 +0000

    [udp6] fix possible panic due to lack of locking.

    The lookup for a IPv6 multicast addresses corresponding to
    the destination address in the datagram is protected by the
    NET_EPOCH section. Access to each PCB is protected by INP_RLOCK
    during comparing. But access to socket's so_options field is
    not protected. And in some cases it is possible, that PCB
    pointer is still valid, but inp_socket is not. The patch wides
    lock holding to protect access to inp_socket. It copies locking
    strategy from IPv4 UDP handling.

    PR:     232192
    Approved by:    re (gjb)
    Obtained from:  Yandex LLC
    Sponsored by:   Yandex LLC
    Differential Revision:  https://reviews.freebsd.org/D28232

    (cherry picked from commit 3c782d9c91666886d868426f0aea040d1a1a8ee4)
    (cherry picked from commit 8cb4c363163062bceec92383eae43f5a32049c73)

 sys/netinet6/udp6_usrreq.c | 61 +++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 33 deletions(-)